Re: separate L3 output KERNEL_LOCK

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

   Date: Thu, 16 Jun 2016 12:26:10 +0900
   From: Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>

   I rewrite my code. Here is patch series,
       http://www.netbsd.org/~knakahara/separate-l3-lock-2/separate-l3-lock-2.tgz
   and here is unified patch.
       http://www.netbsd.org/~knakahara/separate-l3-lock-2/unified-separate-l3-lock-2.patch

   Could you comment this patch?

Looks pretty good!  Some comments from a quick skim:

First, did you catch all the direct uses of ifp->if_output?

I see a few by grepping that don't appear in your patch:

. dist/pf/net/pf.c
. external/bsd/ipf/netinet/ip_fil_netbsd.c
. net/if_ecosubr.c
. netatalk/aarp.c
. netatalk/ddp_output.c

Or is there a reason you don't need to convert those?

Same goes for ifp->if_start.  Here are all the uses I don't see
converted -- but many of these are in drivers which are perhaps
deliberately calling their own start routines and hence maybe don't
need to go through if_start_lock:

. altq/altq_cbq.c
. altq/altq_subr.c
. arch/acorn26/ioc/if_eca.c
. dev/ic/arn5008.c
. dev/ic/arn9003.c
. dev/ic/bwi.c
. dev/pci/if_ipw.c
. dev/pci/if_iwi.c
. dev/pci/if_iwm.c
. dev/pci/if_iwn.c
. dev/pci/if_wm.c
. dev/usb/if_athn_usb.c
. dev/usb/uhso.c
. kern/kern_pmf.c
. net/if_ecosubr.c
. net/if_spppsubr.c

   --- a/sys/dev/pci/if_wm.c
   +++ b/sys/dev/pci/if_wm.c
   @@ -2407,6 +2407,7 @@ alloc_retry:
           strlcpy(ifp->if_xname, xname, IFNAMSIZ);
           ifp->if_softc = sc;
           ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
   +       ifp->if_flags = IFEF_START_MPSAFE;


Should be ifp->if_extflags = IFEF_START_MPSAFE, no?

Also, if there's any chance that ether_ifattach or similar might be
called first (which it isn't in this case but perhaps might be in
other drivers in the future), then maybe this should be `|=' instead
of `=', since ifp->if_extflags is shared between the two layers.

   --- a/sys/net/if.h
   +++ b/sys/net/if.h
   @@ -242,7 +242,8 @@ typedef struct ifnet {
           u_short if_index;               /* numeric abbreviation for this if */
           short   if_timer;               /* time 'til if_slowtimo called */
           short   if_flags;               /* up/down, broadcast, etc. */
   -       short   if__pad1;               /* be nice to m68k ports */
   +       short   if_extflags;            /* device driver MP-safe, etc. */
   +                                       /* be nice to m68k ports */

You can remove the `be nice to m68k ports' comment, I think.  I
presume it was a comment explaining why there is padding there.

   --- a/sys/net/if_ethersubr.c
   +++ b/sys/net/if_ethersubr.c
   @@ -910,6 +919,7 @@ ether_ifattach(struct ifnet *ifp, const uint8_t *lla)
    {
           struct ethercom *ec = (struct ethercom *)ifp;

   +       ifp->if_extflags = IFEF_OUTPUT_MPSAFE;

This should probably use `|=', not `=' -- otherwise I expect it to
overwrite any IFEF_START_MPSAFE flag supplied by the driver!

Just to make sure these assignments don't clobber each other, can you
add KASSERT(ifp->if_extflags & IFEF_OUTPUT_MPSAFE) to ether_output and
KASSERT(ifp->if_extflags & IFEF_START_MPSAFE) to wm_start?  (We
probably can't kassert that the kernel lock is dropped, unfortunately,
but this is a close enough proxy for now.)