On Mon, May 9, 2016 at 6:44 AM, Jesper Dangaard Brouer <bro...@redhat.com> wrote: > It have always been required to call napi_alloc_skb from NAPI/softirq > context, which implies running with local_bh_disable'ed. Thus, this > code path should already be well tested. But recent SKB bulk changes > introduced will make this more volatile and bugs more subtle, if this > is violated.
It hasn't been required to be in the softirq routine, bottom halves could have been disabled. I used the NAPI parameter to try and enforce a strong correlation between the two but I didn't make it a hard requirement. It isn't until you try to do the recycling that it becomes more of a requirement because then it is tied into the bulk free. The bulk free routine had a stronger requirement for NAPI because it needed to have the __kfree_skb_flush routine called to finally free the skbuffs that were still hanging off the percpu skbuff structure. > To catch any driver violating this add a loud WARN_ON. > > Performance wise, I do worry about adding this runtime check code into > the hotpath, of this highly optimized function call. I've > micro-benchmarked it with both IP-forwarding and local UDP delivery, > and didn't see any regressions. It does adds extra code size (icache). > > add/remove: 0/0 grow/shrink: 1/0 up/down: 43/0 (43) > function old new delta > __napi_alloc_skb 461 504 +43 > > Signed-off-by: Jesper Dangaard Brouer <bro...@redhat.com> > --- > net/core/skbuff.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index e85f1065b263..99addbf66f2e 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -498,6 +498,9 @@ struct sk_buff *__napi_alloc_skb(struct napi_struct > *napi, unsigned int len, > struct sk_buff *skb; > void *data; > > + /* Catch drivers violating, not having local BH disabled */ > + WARN_ON(!in_softirq()); > + I'm pretty sure this still lets you get away with just disabling bottom halves. I think what you would want is in_serving_softirq(). > len += NET_SKB_PAD + NET_IP_ALIGN; > > if (unlikely((len > SKB_WITH_OVERHEAD(PAGE_SIZE)) || >
