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
