Re: Lockless IP input queue, the pktqueue interface

[David Young <dyoung%pobox.com@localhost>]

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 <darrenr%netbsd.org@localhost> wrote:
> >>On 29/05/2014 5:06 AM, Mindaugas Rasiukevicius wrote:
> >>>Darren Reed <darrenr%netbsd.org@localhost> 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.

-- 
David Young
dyoung%pobox.com@localhost    Urbana, IL    (217) 721-9981