Hi,
Jussi Kivilinna <jussi.kivilinna@xxxxxxxxxxx> writes:
>> Felipe Balbi <balbi@xxxxxxxxxx> writes:
>>>>>> Enabling SG allows enabling GSO (generic segmentation offload) feature
>>>>>> of linux networking layer. This increases TCP throughput with NCM
>>>>>> on Cortex-A15+USB3380 based device from 300 Mbit/s to 1.1 Gbit/s.
>>>>>>
>>>>>> Signed-off-by: Jussi Kivilinna <jussi.kivilinna@xxxxxxxxxxx>
>>>>>
>>>>> this is AWESOME!! :-) But here's the thing, any chance we can build this
>>>>> in u_ether.c ? Also, NETIF_F_SG should be conditional on
>>>>> gadget->sg_supported so that we don't break UDCs that don't support
>>>>> sglists.
>>>>>
>>>>
>>>> Actually, no sglists are passed to UDC. Reason why this work
>>>> with minimal changes for NCM is that NCM does tx buffering
>>>> in its wrap function.. 'ncm_wrap_ntb' copies input skbuffs to
>>>> larger skbuff, so enabling SG is only matter of changing that
>>>> skbuff data copy from 'memcpy' to 'skb_copy_bits' (and changing
>>>> CRC calculation work with skbuff fragments). Since NCM already
>>>> does copying, SG can be enabled for NCM without extra overhead.
>>>
>>> aha, understood. Now what if we skip copying altogether? If we have an
>>> sg and a UDC that supports sg (gadget->sg_supported = 1), then we can
>>> avoid copying, right?
>>
>> perhaps this is the crud of the change (still need to check for
>> sg_supported)?
>>
>> diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
>> index 5f562c1ec795..f3497cba32ec 100644
>> --- a/drivers/usb/gadget/function/u_ether.c
>> +++ b/drivers/usb/gadget/function/u_ether.c
>> @@ -235,7 +235,7 @@ rx_submit(struct eth_dev *dev, struct usb_request *req, gfp_t gfp_flags)
>> */
>> skb_reserve(skb, NET_IP_ALIGN);
>>
>> - req->buf = skb->data;
>> + req->num_sgs = skb_to_sgvec(skb, req->sg, 0, skb->len);
>> req->length = size;
>> req->complete = rx_complete;
>> req->context = skb;
>>
>
> I did experiment with this, but cannot fully test it without
> sg_supported UDC. Also, wrapper functions need to be reviewed
> to make sure they work with non-linear skbuffs. Seems that EEM
> wrapper had problem with SG, probably because it tries to add
> CRC at the skbuff tail. Below is patch which I've tested for
> now. It applies on top of the earlier NCM SG patch.
>
> -Jussi
>
> ---
> [PATCH] [RFC] usb: gadget: u_ether: enable NETIF_F_SG for networking gadgets
>
> Signed-off-by: Jussi Kivilinna <jussi.kivilinna@xxxxxxxxxxx>
I tested this patch on top of previous two patches. It works fine for a
while, but then starts failing. No idea yet why it fails. I'll try to
debug this for a while longer.
> @@ -363,6 +364,14 @@ static int prealloc(struct list_head *list, struct usb_ep *ep, unsigned n)
> }
> while (i--) {
> req = usb_ep_alloc_request(ep, GFP_ATOMIC);
> + if (alloc_sg && req) {
> + req->sg = kmalloc(sizeof(*req->sg) *
> + (MAX_SKB_FRAGS + 2), GFP_ATOMIC);
how about:
kmalloc_array(MAX_SKB_FRAGS + 2, sizeof(*req->sg), GFP_ATOMIC)
> @@ -376,6 +385,8 @@ extra:
>
> next = req->list.next;
> list_del(&req->list);
> + if (req->sg)
> + kfree(req->sg);
the check here is unnecessary. Just kfree(req->sg) :-)
> @@ -391,10 +402,11 @@ static int alloc_requests(struct eth_dev *dev, struct gether *link, unsigned n)
> int status;
>
> spin_lock(&dev->req_lock);
> - status = prealloc(&dev->tx_reqs, link->in_ep, n);
> + status = prealloc(&dev->tx_reqs, link->in_ep, n,
> + dev->gadget->sg_supported);
> if (status < 0)
> goto fail;
> - status = prealloc(&dev->rx_reqs, link->out_ep, n);
> + status = prealloc(&dev->rx_reqs, link->out_ep, n, false);
why don't we want sglist for rx?
--
balbi
Attachment:
signature.asc
Description: PGP signature
