Date: Wed, 12 Jan 2022 17:47:44 -0700 From: Kevin Bowling <kevin.bowling@kev009.com> To: Alexander Motin <mav@freebsd.org> Cc: Eric Joyner <erj@freebsd.org>, "Galazka, Krzysztof" <krzysztof.galazka@intel.com>, "Joyner, Eric" <eric.joyner@intel.com>, freebsd-net@freebsd.org Subject: Re: iflib_timer() vs ixl_admin_timer() race Message-ID: <CAK7dMtAhEbuoiTyTAP9fmZDnVbmV7gU1rGdcd-BzHUyTpdsS-Q@mail.gmail.com> In-Reply-To: <c7a17766-76a3-1e88-418e-93e95aac7298@FreeBSD.org> References: <27d6f051-3108-a08a-6f1d-670dd8fdb9bb@FreeBSD.org> <CAKdFRZiu4e6vCFSCLh_6c3k20D6Fy5ygtq1-Si9_eucJ=VRPHQ@mail.gmail.com> <fc52b0d2-79f9-9b39-fc41-8c51da2d4e0d@FreeBSD.org> <CAKdFRZiDYpQanLyfixpr4Xm35LqoS56-GJ485UExg_jAqKdACg@mail.gmail.com> <DM8PR11MB559003BBCB3139A3DDB2D928F4529@DM8PR11MB5590.namprd11.prod.outlook.com> <c7a17766-76a3-1e88-418e-93e95aac7298@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--000000000000795f4205d56c0930 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, Jan 12, 2022 at 10:18 AM Alexander Motin <mav@freebsd.org> wrote: > Thanks, Krzystof, > > Grepping now for iflib_admin_intr_deferred() through the sources I see > the same issue in other Intel NIC drivers, plus bnxt(4). So the main > controversy I see is this: either admin intr should not stop and restart > the callouts (and then question is why it does that now), or if it > should be so heavy-weight, it should not be called so regularly (and > then question why so many drivers do it). > > In few drivers I've found it even more interesting: iflib_timer() calls > IFDI_TIMER(), which calls ixgbe_if_timer(), which calls > iflib_admin_intr_deferred(), which in its turn restart the > iflib_timer(). Ouroboros. ;) > >From memory, I believe that call may be related to NICs without dedicated admin intrs. Please keep them in mind when you clean this up. > On 12.01.2022 11:46, Galazka, Krzysztof wrote: > > Hi Alexander, > > > > Thank you for pointing this out. ixl_admin_timer is used by a callout s= o > > I don=E2=80=99t think we can acquire any locks there. IIRC it was added= to let > > tools for NVM update interact with FW while interface is down, so maybe > > stopping it when interface goes UP would be an option. Let me think thi= s > > through. > > > > Thanks, > > > > Krzysiek > > > > *From:* Eric Joyner <erj@freebsd.org> > > *Sent:* Wednesday, January 12, 2022 8:22 AM > > *To:* Alexander Motin <mav@freebsd.org> > > *Cc:* Joyner, Eric <eric.joyner@intel.com>; Galazka, Krzysztof > > <krzysztof.galazka@intel.com> > > *Subject:* Re: iflib_timer() vs ixl_admin_timer() race > > > > Well, I think the situation is more-or-less as you say -- it's just tha= t > > the intent of ixl_admin_timer() is supposed to have the adapter's admin > > queue processed constantly, regardless of interrupt status or down/up > > status. I think as a shorthand we just called _task_fn_admin because it > > handles a bunch of other things as well as getting down to calling > > ixl_if_update_admin_status() which does the admin queue processing. But > > as you found, I don't think it was originally written to be called > > periodically on a regular basis like iflib_timer(), so the callouts are > > interacting badly. > > > > My first thought would be to replace the call to > > iflib_admin_intr_deferred() in ixl_admin_timer() with > > ixl_if_update_admin_status() while taking the CTX_LOCK(). I'm not sure > > everything else in _task_fn_admin() needs to run periodically like that > > in the driver. That would avoid the callouts getting stopped every 500 > > ticks. > > > > I CC'd my coworker Krzysztof who currently owns the driver; he might > > have better thoughts on this. > > > > - Eric > > > > On Tue, Jan 11, 2022 at 10:46 AM Alexander Motin <mav@freebsd.org > > <mailto:mav@freebsd.org>> wrote: > > > > Yes. I've reverted my patch for now to not break anything, but all > > this > > situation does not look right for me too, so I'd appreciate your > look. > > > > On 11.01.2022 12:21, Eric Joyner wrote: > > > Hi, > > > > > > I came back from vacation yesterday, but I'll look into this for > you > > > today. It's not a good situation when changing the period of the > > iflib > > > timer breaks something in the driver... > > > > > > - Eric > > > > > > On Sun, Jan 9, 2022 at 8:19 PM Alexander Motin <mav@freebsd.org > > <mailto:mav@freebsd.org> > > > <mailto:mav@freebsd.org <mailto:mav@freebsd.org>>> wrote: > > > > > > Hi Eric, > > > > > > In 90bc1cf65778 I've done what looked like a trivial > > optimization. But > > > just after committing it I've noticed weird race it created > > between > > > iflib_timer() and ixl_admin_timer() timers. I see ixl(4) > calls > > > iflib_admin_intr_deferred() every 0.5s, which calls > > _task_fn_admin(), > > > which every time stops and restart txq->ift_timer callout (A= KA > > > iflib_timer()), which actually has exactly the same period o= f > > 0.5s. It > > > seems before my change iflib_timer() managed to run once for > > all TX > > > queues before being stopped, but after the change due to > > additional > > > jitter many of callouts are getting stopped before firing. > > > > > > Could you please sched some light for me on what is going on > > there? > > > That race between two callouts looks like potential bug to > > me, working > > > by some coinncedence, especially considering iflib_timer() > > period is > > > tunable. > > > > > > Thanks in advance. > > > > > > -- > > > Alexander Motin > > > > > > > -- > > Alexander Motin > > > > -----------------------------------------------------------------------= - > > *Intel Technology Poland sp. z o.o. > > * ul. S=C5=82owackiego 173 | 80-298 Gda=C5=84sk | S=C4=85d Rejonowy Gda= =C5=84sk P=C3=B3=C5=82noc | VII > > Wydzia=C5=82 Gospodarczy Krajowego Rejestru S=C4=85dowego - KRS 101882 = | NIP > > 957-07-52-316 | Kapita=C5=82 zak=C5=82adowy 200.000 PLN. > > > > Ta wiadomo=C5=9B=C4=87 wraz z za=C5=82=C4=85cznikami jest przeznaczona = dla okre=C5=9Blonego > > adresata i mo=C5=BCe zawiera=C4=87 informacje poufne. W razie przypadko= wego > > otrzymania tej wiadomo=C5=9Bci, prosimy o powiadomienie nadawcy oraz tr= wa=C5=82e > > jej usuni=C4=99cie; jakiekolwiek przegl=C4=85danie lub rozpowszechniani= e jest > > zabronione. > > This e-mail and any attachments may contain confidential material for > > the sole use of the intended recipient(s). If you are not the intended > > recipient, please contact the sender and delete all copies; any review > > or distribution by others is strictly prohibited. > > > > -- > Alexander Motin > > --000000000000795f4205d56c0930 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div><br></div><div><br><div class=3D"gmail_quote"><div dir=3D"ltr" class= =3D"gmail_attr">On Wed, Jan 12, 2022 at 10:18 AM Alexander Motin <<a hre= f=3D"mailto:mav@freebsd.org">mav@freebsd.org</a>> wrote:<br></div><block= quote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left-w= idth:1px;border-left-style:solid;padding-left:1ex;border-left-color:rgb(204= ,204,204)">Thanks, Krzystof,<br> <br> Grepping now for iflib_admin_intr_deferred() through the sources I see <br> the same issue in other Intel NIC drivers, plus bnxt(4).=C2=A0 So the main = <br> controversy I see is this: either admin intr should not stop and restart <b= r> the callouts (and then question is why it does that now), or if it <br> should be so heavy-weight, it should not be called so regularly (and <br> then question why so many drivers do it).<br> <br> In few drivers I've found it even more interesting: iflib_timer() calls= <br> IFDI_TIMER(), which calls ixgbe_if_timer(), which calls <br> iflib_admin_intr_deferred(), which in its turn restart the <br> iflib_timer().=C2=A0 Ouroboros. ;)<br> </blockquote><div dir=3D"auto"><br></div><div dir=3D"auto">From memory, I b= elieve that call may be related to NICs without dedicated admin intrs.=C2= =A0 Please keep them in mind when you clean this up.</div><div dir=3D"auto"= ><br></div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.= 8ex;border-left-width:1px;border-left-style:solid;padding-left:1ex;border-l= eft-color:rgb(204,204,204)"><br> On 12.01.2022 11:46, Galazka, Krzysztof wrote:<br> > Hi Alexander,<br> > <br> > Thank you for pointing this out. ixl_admin_timer is used by a callout = so <br> > I don=E2=80=99t think we can acquire any locks there. IIRC it was adde= d to let <br> > tools for NVM update interact with FW while interface is down, so mayb= e <br> > stopping it when interface goes UP would be an option. Let me think th= is <br> > through.<br> > <br> > Thanks,<br> > <br> > Krzysiek<br> > <br> > *From:* Eric Joyner <<a href=3D"mailto:erj@freebsd.org" target=3D"_= blank">erj@freebsd.org</a>><br> > *Sent:* Wednesday, January 12, 2022 8:22 AM<br> > *To:* Alexander Motin <<a href=3D"mailto:mav@freebsd.org" target=3D= "_blank">mav@freebsd.org</a>><br> > *Cc:* Joyner, Eric <<a href=3D"mailto:eric.joyner@intel.com" target= =3D"_blank">eric.joyner@intel.com</a>>; Galazka, Krzysztof <br> > <<a href=3D"mailto:krzysztof.galazka@intel.com" target=3D"_blank">k= rzysztof.galazka@intel.com</a>><br> > *Subject:* Re: iflib_timer() vs ixl_admin_timer() race<br> > <br> > Well, I think the situation is more-or-less as you say -- it's jus= t that <br> > the intent of ixl_admin_timer() is supposed to have the adapter's = admin <br> > queue processed constantly, regardless of interrupt status or down/up = <br> > status. I think as a shorthand we just called _task_fn_admin because i= t <br> > handles a bunch of other things as well as getting down to calling <br= > > ixl_if_update_admin_status() which does the admin queue processing. Bu= t <br> > as you found, I don't think it was originally written to be called= <br> > periodically on a regular basis like iflib_timer(), so the callouts ar= e <br> > interacting badly.<br> > <br> > My first thought would be to replace the call to <br> > iflib_admin_intr_deferred() in ixl_admin_timer() with <br> > ixl_if_update_admin_status() while taking the CTX_LOCK(). I'm not = sure <br> > everything else in _task_fn_admin() needs to run periodically like tha= t <br> > in the driver. That would avoid the callouts getting stopped every 500= <br> > ticks.<br> > <br> > I CC'd my coworker Krzysztof who currently owns the driver; he mig= ht <br> > have better thoughts on this.<br> > <br> > - Eric<br> > <br> > On Tue, Jan 11, 2022 at 10:46 AM Alexander Motin <<a href=3D"mailto= :mav@freebsd.org" target=3D"_blank">mav@freebsd.org</a> <br> > <mailto:<a href=3D"mailto:mav@freebsd.org" target=3D"_blank">mav@fr= eebsd.org</a>>> wrote:<br> > <br> >=C2=A0 =C2=A0 =C2=A0Yes.=C2=A0 I've reverted my patch for now to no= t break anything, but all<br> >=C2=A0 =C2=A0 =C2=A0this<br> >=C2=A0 =C2=A0 =C2=A0situation does not look right for me too, so I'= d appreciate your look.<br> > <br> >=C2=A0 =C2=A0 =C2=A0On 11.01.2022 12:21, Eric Joyner wrote:<br> >=C2=A0 =C2=A0 =C2=A0 > Hi,<br> >=C2=A0 =C2=A0 =C2=A0 ><br> >=C2=A0 =C2=A0 =C2=A0 > I came back from vacation yesterday, but I= 9;ll look into this for you<br> >=C2=A0 =C2=A0 =C2=A0 > today. It's not a good situation when cha= nging the period of the<br> >=C2=A0 =C2=A0 =C2=A0iflib<br> >=C2=A0 =C2=A0 =C2=A0 > timer breaks something in the driver...<br> >=C2=A0 =C2=A0 =C2=A0 ><br> >=C2=A0 =C2=A0 =C2=A0 > - Eric<br> >=C2=A0 =C2=A0 =C2=A0 ><br> >=C2=A0 =C2=A0 =C2=A0 > On Sun, Jan 9, 2022 at 8:19 PM Alexander Moti= n <<a href=3D"mailto:mav@freebsd.org" target=3D"_blank">mav@freebsd.org<= /a><br> >=C2=A0 =C2=A0 =C2=A0<mailto:<a href=3D"mailto:mav@freebsd.org" targe= t=3D"_blank">mav@freebsd.org</a>><br> >=C2=A0 =C2=A0 =C2=A0 > <mailto:<a href=3D"mailto:mav@freebsd.org"= target=3D"_blank">mav@freebsd.org</a> <mailto:<a href=3D"mailto:mav@fre= ebsd.org" target=3D"_blank">mav@freebsd.org</a>>>> wrote:<br> >=C2=A0 =C2=A0 =C2=A0 ><br> >=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0Hi Eric,<br> >=C2=A0 =C2=A0 =C2=A0 ><br> >=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0In 90bc1cf65778 I've d= one what looked like a trivial<br> >=C2=A0 =C2=A0 =C2=A0optimization.=C2=A0 But<br> >=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0just after committing it I= 've noticed weird race it created<br> >=C2=A0 =C2=A0 =C2=A0between<br> >=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0iflib_timer() and ixl_admi= n_timer() timers.=C2=A0 I see ixl(4) calls<br> >=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0iflib_admin_intr_deferred(= ) every 0.5s, which calls<br> >=C2=A0 =C2=A0 =C2=A0_task_fn_admin(),<br> >=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0which every time stops and= restart txq->ift_timer callout (AKA<br> >=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0iflib_timer()), which actu= ally has exactly the same period of<br> >=C2=A0 =C2=A0 =C2=A00.5s.=C2=A0 It<br> >=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0seems before my change ifl= ib_timer() managed to run once for<br> >=C2=A0 =C2=A0 =C2=A0all TX<br> >=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0queues before being stoppe= d, but after the change due to<br> >=C2=A0 =C2=A0 =C2=A0additional<br> >=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0jitter many of callouts ar= e getting stopped before firing.<br> >=C2=A0 =C2=A0 =C2=A0 ><br> >=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0Could you please sched som= e light for me on what is going on<br> >=C2=A0 =C2=A0 =C2=A0there?<br> >=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0That race between two call= outs looks like potential bug to<br> >=C2=A0 =C2=A0 =C2=A0me, working<br> >=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0by some coinncedence, espe= cially considering iflib_timer()<br> >=C2=A0 =C2=A0 =C2=A0period is<br> >=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0tunable.<br> >=C2=A0 =C2=A0 =C2=A0 ><br> >=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0Thanks in advance.<br> >=C2=A0 =C2=A0 =C2=A0 ><br> >=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0--<br> >=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0Alexander Motin<br> >=C2=A0 =C2=A0 =C2=A0 ><br> > <br> >=C2=A0 =C2=A0 =C2=A0-- <br> >=C2=A0 =C2=A0 =C2=A0Alexander Motin<br> > <br> > ----------------------------------------------------------------------= --<br> > *Intel Technology Poland sp. z o.o.<br> > * ul. S=C5=82owackiego 173 | 80-298 Gda=C5=84sk | S=C4=85d Rejonowy Gd= a=C5=84sk P=C3=B3=C5=82noc | VII <br> > Wydzia=C5=82 Gospodarczy Krajowego Rejestru S=C4=85dowego - KRS 101882= | NIP <br> > 957-07-52-316 | Kapita=C5=82 zak=C5=82adowy 200.000 PLN.<br> > <br> > Ta wiadomo=C5=9B=C4=87 wraz z za=C5=82=C4=85cznikami jest przeznaczona= dla okre=C5=9Blonego <br> > adresata i mo=C5=BCe zawiera=C4=87 informacje poufne. W razie przypadk= owego <br> > otrzymania tej wiadomo=C5=9Bci, prosimy o powiadomienie nadawcy oraz t= rwa=C5=82e <br> > jej usuni=C4=99cie; jakiekolwiek przegl=C4=85danie lub rozpowszechnian= ie jest <br> > zabronione.<br> > This e-mail and any attachments may contain confidential material for = <br> > the sole use of the intended recipient(s). If you are not the intended= <br> > recipient, please contact the sender and delete all copies; any review= <br> > or distribution by others is strictly prohibited.<br> > <br> <br> -- <br> Alexander Motin<br> <br> </blockquote></div></div> --000000000000795f4205d56c0930--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAK7dMtAhEbuoiTyTAP9fmZDnVbmV7gU1rGdcd-BzHUyTpdsS-Q>