Hi, Jussi Kivilinna <jussi.kivili...@haltian.com> writes: >> Felipe Balbi <ba...@kernel.org> 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.kivili...@haltian.com> >>>>> >>>>> 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.kivili...@haltian.com>
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
signature.asc
Description: PGP signature
