Re: separate L3 output KERNEL_LOCK

[Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>]

Hi,

Thank you for your comments.

On 2016/06/16 12:46, Taylor R Campbell wrote:
>    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?

I need to convert them.
# Hmm, I overlooked them while I wrote down my memo.

> 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

Hmm, I think ifp->if_start in arch/* and dev/* are themselves' start
routine, that is, they are not "from L2 to device driver processing".
So, I think it causes confusion to convert such ifp->if_start.

In contrast, altq/* and net/* should be converted. Uh... I overlooked
them.

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

Yes. I update a little while ago.

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

I will remove it.

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

Oops, I change to '|='.

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

Yes, I will add KASSERT appropriately.


From the above, I update the patch series,
    http://www.netbsd.org/~knakahara/separate-l3-lock-2/separate-l3-lock-2.tgz
that is, add 0010 - 0013 patches.
and here is the updated unified patch.
    http://www.netbsd.org/~knakahara/separate-l3-lock-2/unified-separate-l3-lock-2.patch

Could you comment this patch?

Does anyone else have any comments? Any comments are welcome.


Thanks,

-- 
//////////////////////////////////////////////////////////////////////
Internet Initiative Japan Inc.

Device Engineering Section,
IoT Platform Development Department,
Network Division,
Technology Unit

Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>