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-nakah...@iij.ad.jp> > > 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-nakah...@iij.ad.jp>