RE: changing usbnet's API to better deal with cdc-ncm

4 messages, 4 authors, 2012-09-07 · open the first message on its own page

RE: changing usbnet's API to better deal with cdc-ncm

From: Alexey ORISHKO <hidden>
Date: 2012-09-06 08:13:01

-----Original Message-----
From: Oliver Neukum [mailto:oneukum-l3A5Bk7waGM@public.gmane.org]
looking at cdc-ncm it seeems to me that cdc-ncm is forced to play very
dirty games because usbnet doesn't have a notion about aggregating
packets for a single transfer.
Several issues need to be improved:
Tx path:
1. IP packets must be accumulated in one NTB. Currently it's done via data copy.
Preferred way would be a possibility to have a list of skb-s in resulting frame sent down.

2. Timer is needed to accumulate enough packets to get acceptable throughput and reducing amount of HW INTs on device side.
If we get access to Tx queue size and can read skb from it, time can be removed. But Tx queue is above usbnet, so doesn't look possible in current framework.

3. While trying to get best throughput we also need to keep latency in mind, because in some setups it is quite important.
As a simple solution configuration parameters could be used to address this issue.

Rx path:
4. IP packets are cloned to separate skb and length of actual data set to eth packet size, while skb size is still the same as skb containing full NTB frame. This causes problems with TCP stack when throughput is high because of flow control in the stack, if too much data allocated.
Someone suggested a patch for it, which was rejected. Anyway, I don't think copy data to a new skb would be a nice solution, but keeping several clones with size equal to incoming skb in not good either. 

/alexey
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: changing usbnet's API to better deal with cdc-ncm

From: Oliver Neukum <hidden>
Date: 2012-09-06 08:51:34

On Thursday 06 September 2012 10:13:01 Alexey ORISHKO wrote:
quoted
-----Original Message-----
From: Oliver Neukum [mailto:oneukum@suse.de]
quoted
looking at cdc-ncm it seeems to me that cdc-ncm is forced to play very
dirty games because usbnet doesn't have a notion about aggregating
packets for a single transfer.
Several issues need to be improved:
Tx path:
1. IP packets must be accumulated in one NTB. Currently it's done via data copy.
Preferred way would be a possibility to have a list of skb-s in resulting frame sent down.
I am afraid much HCD hardware is just not capable of doing DMA on a list of skbs.
I guess one copy will be necessary. We might consider not freeing the buffer used
to transfer over the bus.
2. Timer is needed to accumulate enough packets to get acceptable throughput and reducing amount of HW INTs on device side.
If we get access to Tx queue size and can read skb from it, time can be removed. But Tx queue is above usbnet, so doesn't look possible in current framework.
I think we can use the number of transfers in flight to make that decision.
So if the choice is to not transfer data at all or to send transfers filled to the
max, we should send, otherwise we ought to accumulate.
That way we get rid of the timer.
3. While trying to get best throughput we also need to keep latency in mind, because in some setups it is quite important.
As a simple solution configuration parameters could be used to address this issue.
I think we should just send the first (two) packages and use the time
to aggregate later packets.
Rx path:
4. IP packets are cloned to separate skb and length of actual data set to eth packet size, while skb size is still the same as skb containing full NTB frame. This causes problems with TCP stack when throughput is high because of flow control in the stack, if too much data allocated.
Someone suggested a patch for it, which was rejected. Anyway, I don't think copy data to a new skb would be a nice solution, but keeping several clones with size equal to incoming skb in not good either. 
Perhaps the problem is using an skb for aggregate reception at all.
Possibly enough buffers of fixed size should be allocated on open and reused,
not freed.

	Regards
		Oliver

Re: changing usbnet's API to better deal with cdc-ncm

From: Ming Lei <tom.leiming@gmail.com>
Date: 2012-09-07 12:58:27

On Thu, Sep 6, 2012 at 4:13 PM, Alexey ORISHKO
[off-list ref] wrote:
quoted
-----Original Message-----
From: Oliver Neukum [mailto:oneukum@suse.de]
quoted
looking at cdc-ncm it seeems to me that cdc-ncm is forced to play very
dirty games because usbnet doesn't have a notion about aggregating
packets for a single transfer.
Several issues need to be improved:
Tx path:
1. IP packets must be accumulated in one NTB. Currently it's done via data copy.
Preferred way would be a possibility to have a list of skb-s in resulting frame sent down.
Looks the copy is inevitable because the buffer length of each skb in the
list is not possible to be max endpoint packet aligned, so one short packet
may terminate the current tx urb transfer.


Thanks,
-- 
Ming Lei

RE: changing usbnet's API to better deal with cdc-ncm

From: Alexey ORISHKO <hidden>
Date: 2012-09-07 13:33:22

-----Original Message-----
From: Ming Lei [mailto:tom.leiming@gmail.com]
Sent: Friday, September 07, 2012 2:58 PM
quoted
Several issues need to be improved:
Tx path:
1. IP packets must be accumulated in one NTB. Currently it's done via
data copy.
quoted
Preferred way would be a possibility to have a list of skb-s in
resulting frame sent down.

Looks the copy is inevitable because the buffer length of each skb in
the list is not possible to be max endpoint packet aligned, so one
short packet may terminate the current tx urb transfer.
Zero or short packet shall not be added if resulting NTB is max negotiated size.
It's yet another problem with usbnet, where we had to add some workarounds
to avoid redundant short/zero packets.

Regards,
Alexey
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help