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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100828151646.GB3211@del.dom.local>
Date:	Sat, 28 Aug 2010 17:16:46 +0200
From:	Jarek Poplawski <jarkao2@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Stephen Hemminger <shemminger@...tta.com>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [RFC] gro: Is it ok to share a single napi from several devs ?

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@...tta.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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html