Re: separate L3 output KERNEL_LOCK

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

Hi,

On 2016/06/16 15:49, Kengo NAKAHARA wrote:
> 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.

Oops, I forget to fix ifnet comment. I add 0014 patch.

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