Re: [RFC] gro: Is it ok to share a single napi from several devs ?

Jarek Poplawski
2010-08-28T15:16:53Z

On Sat, Aug 28, 2010 at 04:48:29PM +0200, Eric Dumazet wrote:
> Le samedi 28 ao??t 2010 ?? 16:31 +0200, Jarek Poplawski a écrit :
> > On Sat, Aug 28, 2010 at 12:54:06PM +0200, Eric Dumazet wrote:
> > > In commit f2bde7328633269ee935d9ed96535ade15cc348f
> > > Author: Stephen Hemminger <shemminger@vyatta.com>
> > >
> > > net: allow multiple dev per napi with GRO
> > >
> > > GRO assumes that there is a one-to-one relationship between NAPI
> > > structure and network device. Some devices like sky2 share multiple
> > > devices on a single interrupt so only have one NAPI handler. Rather than
> > > split GRO from NAPI, just have GRO assume if device changes that
> > > it is a different flow.
> > >
> > >
> > > It was assumed a napi could be shared by several devs, but I dont really
> > > understand, since we have an unique ->dev pointer inside "napi_struct",
> > > this one is set once, and never change.
> > >
> > > This pointer is currently used from napi_get_frags() [but that could be
> > > avoided], and from netpoll_poll_lock().
> > >
> > > The netpoll_poll_lock() case is problematic.
> > >
> > > static inline void *netpoll_poll_lock(struct napi_struct *napi)
> > > {
> > > struct net_device *dev = napi->_dev;
> > >
> > > if (dev && dev->npinfo) {
> > > spin_lock(&napi->poll_lock);
> > >
> > > Maybe we should remove 'dev' field from napi_struct and replace it by a
> > > npinfo pointer ?
> >
> > Sky2 seems to work like a single netdev (with an internal sub-netdev),
> > so I can't see your concern: what is the main aim of this replacement?
>
> I am trying to understand why this commit was needed then.
>
> It adds an extra test in the main loop, testing skb->dev against p->dev,
> it must me for something...
>
> I am trying to say that the one to one relationship between NAPI
> structure and a device is not only a GRO thing, but also a netpoll one.

I guess it's not about the full one to one relationship, but kind of
overloading (pseudo vlan?). How much it's needed is another question,
but at least the order of these tests seems doubtful.

Jarek P.