Re: Lockless IP input queue, the pktqueue interface
Ryota Ozaki ozak...@netbsd.org wrote:
Hi rmind,
maxlen of ip{,6}_pktq cannot be updated via sysctl.
It seems that we need to do it in sysctl_pktq_count
somehow.
Now it can:
http://mail-index.netbsd.org/source-changes/2014/06/09/msg055432.html
--
Mindaugas
Re: Lockless IP input queue, the pktqueue interface
On Mon, Jun 9, 2014 at 10:07 PM, Mindaugas Rasiukevicius
rm...@netbsd.org wrote:
Ryota Ozaki ozak...@netbsd.org wrote:
Hi rmind,
maxlen of ip{,6}_pktq cannot be updated via sysctl.
It seems that we need to do it in sysctl_pktq_count
somehow.
Now it can:
http://mail-index.netbsd.org/source-changes/2014/06/09/msg055432.html
Works fine. Thanks!
ozaki-r
--
Mindaugas
Re: Lockless IP input queue, the pktqueue interface
Hi rmind,
maxlen of ip{,6}_pktq cannot be updated via sysctl.
It seems that we need to do it in sysctl_pktq_count
somehow.
Thanks,
ozaki-r
On Tue, May 27, 2014 at 11:52 AM, Mindaugas Rasiukevicius
rm...@netbsd.org wrote:
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
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().
Extra testing would be helpful.
Thanks.
--
Mindaugas
Re: Lockless IP input queue, the pktqueue interface
On Fri, May 30, 2014 at 09:56:15PM +1000, Darren Reed wrote: I am surprised... no, more like shocked really... that someone as experienced as you are could think this way. Yes, experienced. That means I've seen all manner of code written. And I've never before seen anyone justify a code design decision based on who they want or don't want to change code. Especially when it is open source code. ...he didn't, you know. You made that up and put the words in his mouth. He's following standard practices. As you should be aware. Similarly I've never seen putting structs in .c files lead anywhere useful in the long term. Really? I find that claim equally surprising. And shocking. -- David A. Holland dholl...@netbsd.org
Re: Lockless IP input queue, the pktqueue interface
On 30/05/2014 6:30 AM, David Holland wrote: On Fri, May 30, 2014 at 12:01:23AM +1000, Darren Reed wrote: [code cleanup] All of your arguments boil down to can't trust someone else. Why do you need to be so insulting of other developers in your arguments? Do you think you're the only person capable of making good design decisions? Sorry, but I won't be a party to that kind of attitude and want nothing more to do with this. I am surprised... no, more like shocked really... that someone as experienced as you are could think this way. Yes, experienced. That means I've seen all manner of code written. And I've never before seen anyone justify a code design decision based on who they want or don't want to change code. Especially when it is open source code. Similarly I've never seen putting structs in .c files lead anywhere useful in the long term. I think that Mindaugas is being pragmatic here. Developers are not equally brilliant[*], observant of the rules, or perceptive of the patterns, layers, or abstractions in the code. He is writing the code in a way that discourages us from casually misusing or breaking it by getting under the abstraction. I don't find that offensive. There are other ways to achieve that - as with all programming, there is always more than one solution to a problem. I wouldn't care if he justified it as his preference or how he wants to present the interfaces or he was doing it to reduce bugs (i.e. mrg) or whatever, but justifying it as a means of trying to control who changes code is wrong. Open source code should be written such that it supports others making changes to it and doing so safely and properly. Not to prevent or discourage people. Kind Regards, Darren
Re: Lockless IP input queue, the pktqueue interface
On Thu, May 29, 2014 at 09:46:22AM -0500, David Young wrote: I think that Mindaugas is being pragmatic here. Developers are not equally brilliant[*], observant of the rules, or perceptive of the patterns, layers, or abstractions in the code. He is writing the code in a way that discourages us from casually misusing or breaking it by getting under the abstraction. I don't find that offensive. Indeed, I note that over in tech-kern there is a long running thread in which a user, trying to debug a problem with NetBSD, complains that internals of the cd9660 implementation are *not* properly hidden and are inappropriately exposed in public header files. He seems shocked that we do not go (did not go) much farther to hide implementation details than we do. I tend to agree. We ought to make deliberate decisions about what interfaces we will commit to publishing and expose only those, to the greatest degree possible. Exposing guts datastructures to ease grovelling with kmem seems to me to be exactly the _wrong_ direction to go -- I believe that kmem grovelling is a pernicious practice and should die (from my point of view it represents one of the longest-standing major security issues with NetBSD). Note that implementation hiding does *not* impede source-level debugging as if you've got the source, you've got the guts datastructures. Inappropriately exposing guts also leads to compatibility nightmares and impedes both necessary repair of poorly designed subsystems, and innovation in new ones. If you expose the guts, something _will_ use them, and then you will be forced to either replace (at the least, recompile) a ton of random code when you upgrade the kernel, or maintain complex, nasty, error-prone backwards compatibility code forever. I think we should design the system so we do _not_ have to replace a dozen things in /sbin when we replace the kernel. I certainly do not think we should go in the other direction. Thor
Re: Lockless IP input queue, the pktqueue interface
Hi, Thor Lancelot Simon: Indeed, I note that over in tech-kern there is a long running thread in which a user, trying to debug a problem with NetBSD, complains that internals of the cd9660 implementation are *not* properly hidden Urm, i did not complain but asked about the API/ABI rank of those .h files in /usr/include. And i am not really a user but rather an upstream programmer of an ISO 9660 production program, who wants a decent mount environment for the results of that program. Results so far: 1 user aquired (actually for Xfburn). 3 own packages promoted from wip to pkgsrc. 2 kernel kernel bug fixes and 2 userland bug fixes are committed. 1 bug pending: kern/48815, 1 enhancement pending: kern/48808. 1 obtrusive change proposal to come for supporting large data files. (It is being tested meanwhile.) He seems shocked that we do not go (did not go) much farther to hide implementation details Well, i was surprised to see obvious kernel entrails published. So i asked for instructions about how much care to take with changes. Meanwhile i learned that /usr/src/usr.sbin/makefs is compiling /usr/src/sys/fs/udf/udf_osta.c and /usr/src/usr.sbin/mtree/spec.c So strict encapsulation seems to have never been enforced. On the other hand, as system-neutral userland programmer, i would not dig in the isofs/cd9660/*.h files in order to find some kernel interface on which i could daringly rely. I rather have to take care to keep any system dependency in special adapter modules, so that my code stays portable. Still not decided is whether i shall keep .../cd9660_node.h API-compatible in my change to come. It will cost sizeof(void *) in each ISO 9660 vnode, but on the other hand it saves the work to find any includer or copier of the files which i propose to change. There are such friends of cd9660 and i can hardly propose to break them without providing fixes. But i already see problems with getting reviews for 48808 and for my change-to-come. And most of the known affected programs cannot easily be linked here because of obvious libc incompatibilities between CVS and NetBSD-6.1.3. So i could test only somewhat hacked versions. Note well: I don't complain. It's a do-ocracy. Have a nice day :) Thomas
Re: Lockless IP input queue, the pktqueue interface
I would like to point out that exposing the guts of structures has bitten us many times in the past (FILE, etc.). Once you expose a struct, you are making the size of it known; even if your API does not need it, people might use that fact to keep local copies or declare objects of that type. Maintaining binary compatibility becomes really difficult. On the other hand, hiding a structure can lead to less efficient code, because you need to have getters/setters, allocate memory instead of using the callers stack etc. All these are well-known trade-offs, and given our experience with the structure in discussion in the networking stack (the pollution in device drivers by direct access for example), the path chosen seems the correct one for me. christos
Re: Lockless IP input queue, the pktqueue interface
On 29/05/2014 12:29 PM, Mindaugas Rasiukevicius wrote: Darren Reed darr...@netbsd.org wrote: On 29/05/2014 5:06 AM, Mindaugas Rasiukevicius wrote: Darren Reed darr...@netbsd.org wrote: 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?). 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. Yes, and that is absolutely valid justification. Code review ought to prevent from such problems, but that does not always happen (for a variety of reasons - from developers lacking time, or lack of developers who are familiar with that particular code base, to developers trying to reach a compromise; you know the realities). Technology is also a one of the means to address the human problem, at least partially. Do not get me wrong - I am not advocating black and white world of view. You cannot solve these problems with tough restrictions. Otherwise, one could advocate for adopting something like MISRA C standard; which I think is horrible - it takes away flexibility from an engineer, it forces you to produce ugly and tasteless code. However, it sort of does its job for the purpose it was created for, that is, to be idiot proof. I just think that the BSD community can do better than that. As an old open source we can produce code which is above the average quality. At least I hope so. Despite that, the point i.e. the justification still stands. We just try to apply some common sense when judging which restrictions are worth in our case and which are not. If code review would be so efficient solution, we would not have so much spaghetti code, as we do today. Right? :) Even if on average it is much better than a statistical commercial code. 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. ... It is basically the third phase I described in my original response (and I do use _impl.h headers), except if you read it carefully - you should better have a necessity and provide a good reason for it. Because if you are fiddling with an internal structure - it is a good indicator that you might potentially be doing something wrong; even more so if you fiddle with a lockless data structure as in pktqueue's case. It might be useful some day is a very poor and dangerous reasoning in this case. All of your arguments boil down to can't trust someone else. Why do you need to be so insulting of other developers in your arguments? Do you think you're the only person capable of making good design decisions? Sorry, but I won't be a party to that kind of attitude and want nothing more to do with this. Darren
Re: Lockless IP input queue, the pktqueue interface
On Fri, May 30, 2014 at 12:01:23AM +1000, Darren Reed wrote: On 29/05/2014 12:29 PM, Mindaugas Rasiukevicius wrote: Darren Reed darr...@netbsd.org wrote: On 29/05/2014 5:06 AM, Mindaugas Rasiukevicius wrote: Darren Reed darr...@netbsd.org wrote: 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?). 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. Yes, and that is absolutely valid justification. Code review ought to prevent from such problems, but that does not always happen (for a variety of reasons - from developers lacking time, or lack of developers who are familiar with that particular code base, to developers trying to reach a compromise; you know the realities). Technology is also a one of the means to address the human problem, at least partially. Do not get me wrong - I am not advocating black and white world of view. You cannot solve these problems with tough restrictions. Otherwise, one could advocate for adopting something like MISRA C standard; which I think is horrible - it takes away flexibility from an engineer, it forces you to produce ugly and tasteless code. However, it sort of does its job for the purpose it was created for, that is, to be idiot proof. I just think that the BSD community can do better than that. As an old open source we can produce code which is above the average quality. At least I hope so. Despite that, the point i.e. the justification still stands. We just try to apply some common sense when judging which restrictions are worth in our case and which are not. If code review would be so efficient solution, we would not have so much spaghetti code, as we do today. Right? :) Even if on average it is much better than a statistical commercial code. 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. ... It is basically the third phase I described in my original response (and I do use _impl.h headers), except if you read it carefully - you should better have a necessity and provide a good reason for it. Because if you are fiddling with an internal structure - it is a good indicator that you might potentially be doing something wrong; even more so if you fiddle with a lockless data structure as in pktqueue's case. It might be useful some day is a very poor and dangerous reasoning in this case. All of your arguments boil down to can't trust someone else. Why do you need to be so insulting of other developers in your arguments? Do you think you're the only person capable of making good design decisions? Sorry, but I won't be a party to that kind of attitude and want nothing more to do with this. I think that Mindaugas is being pragmatic here. Developers are not equally brilliant[*], observant of the rules, or perceptive of the patterns, layers, or abstractions in the code. He is writing the code in a way that discourages us from casually misusing or breaking it by getting under the abstraction. I don't find that offensive. Dave [*] However, we are all above average at NetBSD.
re: Lockless IP input queue, the pktqueue interface
I think that Mindaugas is being pragmatic here. Developers are not equally brilliant[*], observant of the rules, or perceptive of the patterns, layers, or abstractions in the code. He is writing the code in a way that discourages us from casually misusing or breaking it by getting under the abstraction. I don't find that offensive. i agree 100%. infact, i spent a few weeks earlier this year making every struct that isn't used as part of a direct interface (like, eg struct tm) in an program written in 1990 hidden inside the relevant source. in the process i found about a dozen bugs, a couple that had evaded me for quite a while, and introduced 4 that were fairly quickly fixed.. (that i know about :-) not *all* code can have structures hidden, but if it is easy to do, it is almost certainly the right solution. it encourages modular code by giving you easy to see boundaries. it shouldn't be a hard-fast rule, which is why pragmatic is great way to describe what rmind is saying. regardless of how brilliant developers are, i want my code to be written like this anyway. it helps *everyone*. .mrg.
Re: Lockless IP input queue, the pktqueue interface
On Fri, May 30, 2014 at 12:01:23AM +1000, Darren Reed wrote: [code cleanup] All of your arguments boil down to can't trust someone else. Why do you need to be so insulting of other developers in your arguments? Do you think you're the only person capable of making good design decisions? Sorry, but I won't be a party to that kind of attitude and want nothing more to do with this. I am surprised... no, more like shocked really... that someone as experienced as you are could think this way. -- David A. Holland dholl...@netbsd.org
Re: Lockless IP input queue, the pktqueue interface
On Fri 30 May 2014 at 05:46:37 +1000, matthew green wrote: regardless of how brilliant developers are, i want my code to be written like this anyway. it helps *everyone*. Sometimes I say, on the topic of code, if I don't understand it, it's probably wrong. That's not because I'm so brilliant, but because code should be clear enough to be understood by average programmers. Code is twice as difficult to debug as to write (citation needed!). So if somebody writes brilliantly complex code but it needs to be debugged later, then nobody can do it. And it is still possible that the reason that I don't understand some piece of code is that the author had a mixed up thought process. -Olaf. -- ___ Olaf 'Rhialto' Seibert -- The Doctor: No, 'eureka' is Greek for \X/ rhialto/at/xs4all.nl-- 'this bath is too hot.' pgpsr5VNMpuDW.pgp Description: PGP signature
Re: Lockless IP input queue, the pktqueue interface
On 29/05/2014 5:06 AM, Mindaugas Rasiukevicius wrote: Darren Reed darr...@netbsd.org wrote: 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?). 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
Re: Lockless IP input queue, the pktqueue interface
On Thu, May 29, 2014 at 08:12:35AM +1000, Darren Reed wrote: 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.
Re: Lockless IP input queue, the pktqueue interface
Darren Reed darr...@netbsd.org wrote: On 29/05/2014 5:06 AM, Mindaugas Rasiukevicius wrote: Darren Reed darr...@netbsd.org wrote: 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?). 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. Yes, and that is absolutely valid justification. Code review ought to prevent from such problems, but that does not always happen (for a variety of reasons - from developers lacking time, or lack of developers who are familiar with that particular code base, to developers trying to reach a compromise; you know the realities). Technology is also a one of the means to address the human problem, at least partially. Do not get me wrong - I am not advocating black and white world of view. You cannot solve these problems with tough restrictions. Otherwise, one could advocate for adopting something like MISRA C standard; which I think is horrible - it takes away flexibility from an engineer, it forces you to produce ugly and tasteless code. However, it sort of does its job for the purpose it was created for, that is, to be idiot proof. I just think that the BSD community can do better than that. As an old open source we can produce code which is above the average quality. At least I hope so. Despite that, the point i.e. the justification still stands. We just try to apply some common sense when judging which restrictions are worth in our case and which are not. If code review would be so efficient solution, we would not have so much spaghetti code, as we do today. Right? :) Even if on average it is much better than a statistical commercial code. 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. ... It is basically the third phase I described in my original response (and I do use _impl.h headers), except if you read it carefully - you should better have a necessity and provide a good reason for it. Because if you are fiddling with an internal structure - it is a good indicator that you might potentially be doing something wrong; even more so if you fiddle with a lockless data structure as in pktqueue's case. It might be useful some day is a very poor and dangerous reasoning in this case. You have to provide a convincing argument to expose a structure, not the other way round. Exposing is trivial and we will always be able to do that, but making it opaque again might be a tremendous job (somebody sent an email today to freebsd-arch on the roadmap for struct ifnet - what an incidental illustration!). P.S. We have many interfaces which keep structures in .c and it is off-topic to this thread; if you want continue with this philosophical discussion, please create a new thread. -- Mindaugas
