Messages in this thread | | | Subject | Re: [PATCH v6 net-next 1/5] net: add napi_id and hash | From | Ben Hutchings <> | Date | Wed, 29 May 2013 21:09:52 +0100 |
| |
On Wed, 2013-05-29 at 09:39 +0300, Eliezer Tamir wrote: > Adds a napi_id and a hashing mechanism to lookup a napi by id. > This will be used by subsequent patches to implement low latency > Ethernet device polling. > Based on a code sample by Eric Dumazet. > > Signed-off-by: Eliezer Tamir <eliezer.tamir@linux.intel.com> [...] > --- a/net/core/dev.c > +++ b/net/core/dev.c [...] > @@ -4136,6 +4143,53 @@ void napi_complete(struct napi_struct *n) > } > EXPORT_SYMBOL(napi_complete); > > +void napi_hash_add(struct napi_struct *napi) > +{ > + if (!test_and_set_bit(NAPI_STATE_HASHED, &napi->state)) { > + > + spin_lock(&napi_hash_lock); > + > + /* 0 is not a valid id */ > + napi->napi_id = 0; > + while (!napi->napi_id) > + napi->napi_id = ++napi_gen_id;
Suppose we're loading/unloading one driver repeatedly while another one remains loaded the whole time. Then once napi_gen_id wraps around, the same ID can be assigned to multiple contexts.
So far as I can see, assigning the same ID twice will just make polling stop working for one of the NAPI contexts; I don't think it causes a crash. And it is exceedingly unlikely to happen in production. But if you're going to the trouble of handling wrap-around at all, you'd better handle this.
[...] > +/* must be called under rcu_read_lock(), as we dont take a reference */ > +struct napi_struct *napi_by_id(int napi_id) > +{ > + unsigned int hash = napi_id % HASH_SIZE(napi_hash); [...]
napi_id should be declared unsigned int here, as elsewhere. The division can't actually yield a negative result because HASH_SIZE() has type size_t and napi_id is promoted to match, but I had to go and look at hashtable.h to check that.
Ben.
-- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked.
| |