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 <[email protected]> >> >> 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 <[email protected]>
