Lockless IP input queue, the pktqueue interface

[Darren Reed]

Hello,
As we are trying to bring more parallelism in our network stack, I would
like to make IPv4 and IPv6 input queues lockless. This is implemented by
replacing struct ifqueue and macros around it with a pktqueue interface.
This interface also abstracts and handles network ISR scheduling and that
will allow us to easily add receiver-side packet steering later.
http://www.netbsd.org/~rmind/ip_pktq.diff
- 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
use enums rather than #define's for things like the counters
if percpu_alloc returns NULL (and it can), will percpu_free panic or
will the kernel panic before then?


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Mindaugas Rasiukevicius]

Post by Darren Reed
<...>

- 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.

Post by Darren Reed
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).

Post by Darren Reed
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.

[Thor Lancelot Simon]

Post by Mindaugas Rasiukevicius

Post by Darren Reed
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).

Enums make debugging a little nicer, sometimes.

Thor

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Darren Reed]

Post by Mindaugas Rasiukevicius

Post by Darren Reed
<...>

- 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.

So if someone were to write a program to grovel through a crash dump
or /dev/mem and print out these structures, how would they get the
definition of it?

Darren



--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Darren Reed]

Post by Thor Lancelot Simon

Post by Mindaugas Rasiukevicius

Post by Darren Reed
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).

Enums make debugging a little nicer, sometimes.

Yeah, they don't work so well for flags but for constants that are never
combined with others, they're good.

Darren


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Mindaugas Rasiukevicius]

Post by Darren Reed

Post by Mindaugas Rasiukevicius
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.

So if someone were to write a program to grovel through a crash dump
or /dev/mem and print out these structures, how would they get the
definition of it?

This is getting off-topic, but for the sake of wondering readers:

Serialize and export the structure, or create a wrapper structure used
for data transportation, or implement interface with accessors/mutators.
If you think that you should be able to fiddle with any structure in the
kernel (as it would be 1980s) then you are plain wrong (do I really need
to explain this?).

[Brett Lymn]

Post by Darren Reed
The method that I've seen used in Solaris (for example) is to use
foo_impl.h to providethe details of data structure that are essentially
private and those .h filesmay or may notbe shipped as part of the end
user system.Using pktqueue_private.h might be suitable.

curses.h already does this, for userland the WINDOW (and other) pointers
are opaque handles. The actual definition is hidden in curses_private.h
which is not installed.
--
Brett Lymn
This email has been sent on behalf of one of the following companies within the BAE Systems Australia group of companies:

BAE Systems Australia Limited - Australian Company Number 008 423 005
BAE Systems Australia Defence Pty Limited - Australian Company Number 006 870 846
BAE Systems Australia Logistics Pty Limited - Australian Company Number 086 228 864

Our registered office is Evans Building, Taranaki Road, Edinburgh Parks,
Edinburgh, South Australia, 5111. If the identity of the sending company is
not clear from the content of this email please contact the sender.

This email and any attachments may contain confidential and legally
privileged information. If you are not the intended recipient, do not copy or
disclose its content, but please reply to this email immediately and highlight
the error to the sender and then immediately delete the message.


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Darren Reed]

Post by Mindaugas Rasiukevicius

Post by Darren Reed

Post by Mindaugas Rasiukevicius
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.

So if someone were to write a program to grovel through a crash dump
or /dev/mem and print out these structures, how would they get the
definition of it?

Serialize and export the structure, or create a wrapper structure used
for data transportation, or implement interface with accessors/mutators.
If you think that you should be able to fiddle with any structure in the
kernel (as it would be 1980s) then you are plain wrong (do I really need
to explain this?).

Your justification for including the structure in the .c file is that
developers can't betrusted to not use header files that clearly aren't
public interfaces. That's what code review is for and puts the focus of
what you're arguing as being a "human problem" and not a "technology
problem." You can't solve "human problems" with technology. Putting the
structure there won't stop determined people, it will just make it harder
and they'll begrudge NetBSD for making their life harder.

Putting structures inside a .c file represents a very short term view
of how the implementation willevolve and be used.

The method that I've seen used in Solaris (for example) is to use
foo_impl.h to providethe details of data structure that are essentially
private and those .h filesmay or may notbe shipped as part of the end
user system.Using pktqueue_private.h might be suitable.

Arguing that idealism from a certain period of time is no longer valid
requires invoking another form of idealism and leaving behind being
pragmatic.

But if you're dead set on taking this approach, feel free to ignore
my suggestions and what I've seen done in other platforms. I can't
stop you any more than I can stop it raining.

Darren


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Masanobu SAITOH]

Hi, rmind.

Hello,
As we are trying to bring more parallelism in our network stack, I would
like to make IPv4 and IPv6 input queues lockless. This is implemented by
replacing struct ifqueue and macros around it with a pktqueue interface.
This interface also abstracts and handles network ISR scheduling and that
will allow us to easily add receiver-side packet steering later.
http://www.netbsd.org/~rmind/ip_pktq.diff
- 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().
Extra testing would be helpful.
Thanks.

Is the change "New interface for the ifnet queue" in the
following page's table?

http://www.netbsd.org/~rmind/smpnet/

The implementaion column of "New interface for the ifnet queue"
is blank :)