On Thu, Sep 6, 2012 at 4:30 PM, Bjørn Mork <bjorn@xxxxxxx> wrote: > Ming Lei <tom.leiming@xxxxxxxxx> writes: >> On Thu, Sep 6, 2012 at 4:12 AM, Oliver Neukum <oneukum@xxxxxxx> wrote: >>> Hi, >>> >>> 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. >> >> The Ethernet API we are using does not support transmitting multiple Ethernet >> frames in a single call, so the aggregation things should be done inside low >> level driver, in fact it is just what some wlan(802.11n) drivers have >> been doing. >> >> IMO, the current .tx_fixup is intelligent enough to handle aggregation: >> >> - return a skb_out for sending if low level drivers think it is >> ready to send >> the aggregated frames >> - return NULL if the low level drivers think they need to wait >> for more frames >> >> Of course, the low level drivers need to start a timer to trigger sending >> remainder frames in case of timeout and no further frames come from >> upper protocol stack. >> >> Looks the introduced .tx_bundle is not necessary since .tx_fixup is OK. > > The minidriver does not have any information about tx in progress. The Inside .tx_fixup, the low level driver will get the tx progress information. > strategy above, which is what cdc_ncm uses today, is fine as long as you > always want to wait as long as you always want to fill as many frames as > possible in each packet. But what if the queue is empty and the device For cdc_ncm, the wait time is controlled by cdc_ncm driver, see cdc_ncm_tx_timeout_start(). > is just sitting there doing nothing while you are waiting for more > frames? Then you are just adding unnecessary latency. As said above, cdc_ncm can control the latency by itself. > > There are also several usbnet minidrivers for protocols with frame > aggregation support. Most of them do not currently implement tx > aggregation, possibly due to the complexity. This makes a common > aggregation framework interesting in any case. Reimplementing similar > loginc in multiple minidrivers is meaningless. If we can abstract some common things about aggregation, it should be meaningful. As far as I know, most of aggregation protocol is very different, so almost all aggregation work is only done by low level driver, such as cdc_ncm. If we want to implement some aggregation framework, maybe below is one solution, correct me if it is wrong. usbnet_start_xmit(): to_send = info->tx_bundle(in_skb, &out_skb) if (!to_send) { usbnet_tx_timeout_start(usbnet); goto not_drop; } skb = info->tx_fixup(dev, out_skb, GFP_ATOMIC); ... Looks the above is no much advantage than doing bundling in .tx_fixup just like cdc_ncm. > > I believe Oliver is adding very useful functionality to usbnet here. > Functionality which at least cdc_ncm and possibly other existing > minidrivers can take advantage of immediately. There also seems to be a Sorry, I don't know other minidrivers, and is there some common things in building aggregation with cdc_ncm? Considered that the patch doesn't implement wait_for_more, suggest Oliver to post a new one with basic wait_for_more support for further discussion. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html