Re: Lockless IP input queue, the pktqueue interface

[Mindaugas Rasiukevicius <rmind%netbsd.org@localhost>]

Darren Reed <darrenr%netbsd.org@localhost> wrote:
> <...>
> > The patch makes the following changes:
> > - Implements pktqueue interface (see pktqueue.{c,h} files).
> > - Replaces ipintrq and ip6intrq with the pktqueue mechanism.
> > - Eliminates kernel-lock from ipintr() and ip6intr().
> > - Some preparation work to push softnet_lock out of ipintr().
> >
> 
> Some general feedback...
> 
> push the struct into the .h file, maybe create two .h files, one that is 
> exposed and one that is internal

No, there is no need to expose the structure.  Even if there would be
another internal component using the structure(s) one should consider
accessors/mutators.  Even if that component would have a good reason
not to use accessors/mutators, the structure should be placed under
#ifdef __SUBSYSTEM_PRIVATE (and certainly #ifdef _KERNEL).  However,
I strongly discourage to start from the last step without having a
necessity first.

One of the main reasons why we ended up with our network stack being
such a mess is exactly this: the internal structures are exposed and
accessed all over the place, there is a lack of strict interfacing,
and the information hiding principle is not followed.

> use enums rather than #define's for things like the counters

You have enums in the public interface.  I do not see a big deal,
though (we still often use #defines even in the new code).

> if percpu_alloc returns NULL (and it can), will percpu_free panic or 
> will the kernel panic before then?
> 

Generally, during the early boot such allocations do not fail and it
is okay to KASSERT() for that.  However, I agree that it should be a
check here; as an interface pktq_create() lets the caller decide when
it is called and what to do.  I will add a check.

-- 
Mindaugas