svn commit: r186187 - head/sys/net

[Robert Watson]

On Tue, 16 Dec 2008, Max Laier wrote:

>> Log:
>>   A few locking fixes and cleanups to pfil hook registration,
>>   unregistration, and execution:
>>
>>   - Add some brackets for clarity and trim a bit of vertical whitespace.
>>   - Remove comments that may not contribute to clarity, such as "Lock"
>>     before acquiring a lock and "Get memory" before allocating memory.
>
> These were meant as sectioning comments as pfil_add_hook grew larger and 
> larger in order to allow WAITOK allocations.  Should probably have been "/* 
> 1. get memory */" etc.

I used to comment a bit more gratuitously along those lines, but bde seems to 
have (at least partially) broken me of the habit.  I felt sure there was a 
note in style(9) on not commenting on obvious things, but the closest I found 
was this:

              exit(EX_OK);    /*
                               * Avoid obvious comments such as
                               * "Exit 0 on success."
                               */

I think, generally, that I agree with the observation that code comments 
should contribute to greater understanding of the code by summarizing complex 
code in a simple way, or lending insight to non-obvious implications of code, 
and that simply mirroring of code behavior at a low level in comments doesn't 
contribute a great deal.

Speaking of PFIL_WAITOK, I actually had a pfil_flags-related question for you: 
I notice that pfil on NetBSD saves and propagates pfil_flags from handler to 
the packet_filter_hook, but on FreeBSD, the flags field is unused.  Is that 
intentional?  If so, I was thinking of putting a /* Unused. */ comment in the 
definition of packet_filter_hook.

Also, I notice PFIL_TYPE_IFNET is defined but not implemented -- did you plan 
to implement it at some point?

>>   - During hook registration, don't drop pfil_list_lock between checking
>>     for a duplicate and registering the hook, as this leaves a race
>>     condition by failing to enforce the "no duplicate hooks" invariant.
>>   - Don't lock the hook during registration, since it's not yet in use.
>
> Shouldn't the WLOCK be obtained regardless in order to obtain a memory 
> barrier for the reading threads?  pfil_run_hooks doesn't interact with the 
> LIST_LOCK so it's unclear in what state the hook head will be when the hook 
> head is first read.

Hmm.  Interesting observation.  However, that approach is not consistent with 
lots of other code, so possibly that other code needs to change as well.  For 
example, ip_init() initializes lots of other global data structures, including 
the fragment reassembly queue and protocol switch, without acquiring locks in 
such a way as to force visibility on other CPUs.

I've CC'd John Baldwin, who might be able to comment on this issue more 
generally.  Should we be, for example, calling { IPQ_LOCK(); IPQ_UNLOCK(); } 
after initializing the IP reassembly queues to make sure that initialization 
is visible to other CPUs that will be calling IPQ_LOCK() before using it?

>>   - Document assumption that hooks will be quiesced before being
>>     unregistered.
>
> This should probably go to pfil(9), too.

Yes, it should--similar notes are probably missing from a number of other 
similar frameworky sorts of things.

>>   - Don't write-lock hooks during removal because they are assumed
>>     quiesced.
>
> Again, this lock also ensures that we see all the changes done in other 
> threads to the pfil head recently - otherwise we might leak a hook function 
> pointer or even fault due to list inconsistencies.  Again, add/del_hook 
> don't interact with the LIST_LOCK.

We may run into similar problems with VIMAGE destructors now that we are 
interested in tearing down network stacks.  Again, perhaps jhb can opine some.

Robert N M Watson
Computer Laboratory
University of Cambridge