On Sat, 8 Dec 2018 11:26:56 -0800 Eric Dumazet <eric.duma...@gmail.com> wrote:
> On 12/08/2018 06:57 AM, Ilias Apalodimas wrote: > > Hi Eric, > >>>> This patch is changing struct sk_buff, and is thus per-definition > >>>> controversial. > >>>> > >>>> Place a new member 'mem_info' of type struct xdp_mem_info, just after > >>>> members (flags) head_frag and pfmemalloc, And not in between > >>>> headers_start/end to ensure skb_copy() and pskb_copy() work as-is. > >>>> Copying mem_info during skb_clone() is required. This makes sure that > >>>> pages are correctly freed or recycled during the altered > >>>> skb_free_head() invocation. > >>> > >>> I read this to mean that this 'info' isn't accessed/needed until skb > >>> is freed. Any reason its not added at the end? > >>> > >>> This would avoid moving other fields that are probably accessed > >>> more frequently during processing. > >>> > >> > >> But I do not get why the patch is needed. > >> > >> Adding extra cost for each skb destruction is costly. > >> > >> I though XDP was all about _not_ having skbs. > > > > You hit the only point i don't personally like in the code, xdp info in the > > skb. Considering the benefits i am convinced this is ok and here's why: > > > >> Please let's do not slow down the non XDP stack only to make XDP more > >> appealing. > > > > We are not trying to do that, on the contrary. The patchset has nothing > > towards > > speeding XDP and slowing down anything else. The patchset speeds up the > > mvneta driver on the default network stack. The only change that was needed > > was > > to adapt the driver to using the page_pool API. The speed improvements we > > are > > seeing on specific workloads (i.e 256b < packet < 400b) are almost 3x. > > > > Lots of high speed drivers are doing similar recycling tricks themselves > > (and > > there's no common code, everyone is doing something similar though). All we > > are > > trying to do is provide a unified API to make that easier for the rest. > > Another > > advantage is that if the some drivers switch to the API, adding XDP > > functionality on them is pretty trivial. > > > > Moreover our tests are only performed on systems without or with SMMU > > disabled. > > On a platform i have access to, enabling and disabling the SMMU has some > > performance impact. By keeping the buffers mapped we believe this impact > > will be substantially less (i'll come back with results once i have them on > > this). > > > > I do understand your point, but the potential advantages on my head > > overwight that by a lot. > > > > I got other concerns on the patchset though. Like how much memory is it > > 'ok' to > > keep mapped keeping in mind we are using the streaming DMA API. Are we > > going to > > affect anyone else negatively by doing so ? > > > > I want to make sure you guys thought about splice() stuff, and > skb_try_coalesce(), and GRO, and skb cloning, and ... Thanks for the pointers. To Ilias, we need to double check skb_try_coalesce() code path, as it does look like we don't handle this correctly. > I do not see how an essential property of page fragments would be > attached to sk_buff, without adding extra code all over the places. > > Page recycling in drivers is fine, but should operate on pages that > the driver owns. > I think we have addressed this. We are only recycling pages that the driver owns. In case of fragments, then we release the DMA-mapping and don't recycle the page, instead the normal code path is taken (which is missing in case of skb_try_coalesce). -- Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer