Re: wm_intr may lead wm_start unexpectedly
On Wed, Jun 25, 2014 at 2:54 PM, Matt Thomas wrote: > > On Jun 24, 2014, at 10:43 PM, Ryota Ozaki wrote: > >> Hi, >> >> I found a strange behavior of if_wm that >> its interrupt handler may call its if_start >> (xmit function) eventually. I don't think >> it's sane. It makes difficult to use mutex for MP. >> >> Here is a call trace: >> wm_intr => wm_linkintr => wm_linkintr_gmii => >> mii_pollstat => makphy_service => mii_phy_update => >> if_link_state_change => in6_if_link_up => >> nd6_dad_start => nd6_dad_ns_output => ... => >> => ether_output => ... => wm_start >> >> The interrupt handler calls mii, mii notifies >> a link state change to inet6, and inet6 tries DAD. >> This IPv6 DAD code (nd6_dad_start and >> nd6_dad_ns_output) is the main issue of my claim. >> nd6_dad_start normally sets up a callout for >> nd6_dad_ns_output, however, it may call >> nd6_dad_ns_output directly at random. > > Simple, set IFF_OACTIVE before calling mii Yeah, that's a simple solution for the mii problem :) What about the DAD's random behavior? Thanks, ozaki-r
Re: wm_intr may lead wm_start unexpectedly
On Jun 24, 2014, at 10:43 PM, Ryota Ozaki wrote: > Hi, > > I found a strange behavior of if_wm that > its interrupt handler may call its if_start > (xmit function) eventually. I don't think > it's sane. It makes difficult to use mutex for MP. > > Here is a call trace: > wm_intr => wm_linkintr => wm_linkintr_gmii => > mii_pollstat => makphy_service => mii_phy_update => > if_link_state_change => in6_if_link_up => > nd6_dad_start => nd6_dad_ns_output => ... => > => ether_output => ... => wm_start > > The interrupt handler calls mii, mii notifies > a link state change to inet6, and inet6 tries DAD. > This IPv6 DAD code (nd6_dad_start and > nd6_dad_ns_output) is the main issue of my claim. > nd6_dad_start normally sets up a callout for > nd6_dad_ns_output, however, it may call > nd6_dad_ns_output directly at random. Simple, set IFF_OACTIVE before calling mii
wm_intr may lead wm_start unexpectedly
Hi, I found a strange behavior of if_wm that its interrupt handler may call its if_start (xmit function) eventually. I don't think it's sane. It makes difficult to use mutex for MP. Here is a call trace: wm_intr => wm_linkintr => wm_linkintr_gmii => mii_pollstat => makphy_service => mii_phy_update => if_link_state_change => in6_if_link_up => nd6_dad_start => nd6_dad_ns_output => ... => => ether_output => ... => wm_start The interrupt handler calls mii, mii notifies a link state change to inet6, and inet6 tries DAD. This IPv6 DAD code (nd6_dad_start and nd6_dad_ns_output) is the main issue of my claim. nd6_dad_start normally sets up a callout for nd6_dad_ns_output, however, it may call nd6_dad_ns_output directly at random. Looking at (1), in6_if_link_up picks a random delay (in tick) ranging from 0 to hz and passes it to nd6_dad_start. nd6_dad_start sets up a callout if the delay > 0 while calls nd6_dad_ns_output directly if the delay == 0 (2). This random behavior makes debugging difficult. OpenBSD seems to have changed nd6_dad_start to always call a callout regardless of the delay (3). I think we should do such an approach as that. By doing so, we can avoid the above wm_intr => wm_start problem. My proposal to fix the problem of nd6_dad_start is to allow a negative value of the delay to indicate that no delay is required and nd6_dad_ns_output is called directly. For any other value including zero, a callout is always used. Any comments? BTW, first of all, mii_pollstat shouldn't lead if_link_state_change and use a callout or softint for further packet xmits. I'm not sure how to deal with it. (1) http://nxr.netbsd.org/xref/src/sys/netinet6/in6.c#2166 (2) http://nxr.netbsd.org/xref/src/sys/netinet6/nd6_nbr.c#1167 (3) http://fxr.watson.org/fxr/source/netinet6/nd6_nbr.c?v=OPENBSD#L1146 ozaki-r
Re: unification of boot.cfg parsing copies
In article <20140624172320.GA930@calculon>, Tyler Retzlaff wrote: >hello, > >presently there are three not-so-exact copies of code for parsing boot.cfg >across i386, sparc and zaurus. > >linked is a patch that attempts to unify the three copies of this code >into a single mi file (bootcfg.c). > >the primary source for the parsing logic was taken from i386 and had >minor tweaks made to it to support what was being done in both zaurus >and sparc. notably the ability to limit the size of the configuration >to be buffered and simply handle and add arch specific with generic >fallback key=value directives. > >any issues with the parsing logic as they stand have not been addressed >as it seemed more reasonable to reduce the number of copies down to >1 before fixing such issues. some name cleanups have been made since >some of the original naming was ambiguious and not grep friendly >i.e. 'bootconf' -> 'bootcfg'. > >note that the previous boot.cfg format used for sparc64 was undocumented >and not compatible. discussion with martin@ identified that boot.cfg >was likely only used by him and it was satisfactory to align it with i386. > >martin@ has been kind enough to test on sparc64 with no obvious defects >and i am also using it on amd64, if someone has a zaurus feel free to >step forward. > >http://www.netbsd.org/~rtr/bootcfg0.diff Looks good at first glance. I would change all exported names to start with bootcfg_ and BOOTCFG_; most of them do already. christos
Re: unification of boot.cfg parsing copies
On Wed, Jun 25, 2014 at 03:23:22AM +1000, Tyler Retzlaff wrote: > note that the previous boot.cfg format used for sparc64 was undocumented > and not compatible. discussion with martin@ identified that boot.cfg > was likely only used by him and it was satisfactory to align it with i386. Not exactly by me, but the config that was put onto the boot CDs, so I assumed the only usage to be the code in-tree (and fixed by Tyler's patch). The previous support in ofwboot was very limited, so other uses are unlikely (and if existing: easy to fix) Martin
unification of boot.cfg parsing copies
hello, presently there are three not-so-exact copies of code for parsing boot.cfg across i386, sparc and zaurus. linked is a patch that attempts to unify the three copies of this code into a single mi file (bootcfg.c). the primary source for the parsing logic was taken from i386 and had minor tweaks made to it to support what was being done in both zaurus and sparc. notably the ability to limit the size of the configuration to be buffered and simply handle and add arch specific with generic fallback key=value directives. any issues with the parsing logic as they stand have not been addressed as it seemed more reasonable to reduce the number of copies down to 1 before fixing such issues. some name cleanups have been made since some of the original naming was ambiguious and not grep friendly i.e. 'bootconf' -> 'bootcfg'. note that the previous boot.cfg format used for sparc64 was undocumented and not compatible. discussion with martin@ identified that boot.cfg was likely only used by him and it was satisfactory to align it with i386. martin@ has been kind enough to test on sparc64 with no obvious defects and i am also using it on amd64, if someone has a zaurus feel free to step forward. http://www.netbsd.org/~rtr/bootcfg0.diff thanks
