From nobody Thu Jan 13 00:47:44 2022 X-Original-To: freebsd-net@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 971CE194C9F8 for ; Thu, 13 Jan 2022 00:47:56 +0000 (UTC) (envelope-from kevin.bowling@kev009.com) Received: from mail-yb1-xb2b.google.com (mail-yb1-xb2b.google.com [IPv6:2607:f8b0:4864:20::b2b]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4JZ5Pm3Zxkz3F6n for ; Thu, 13 Jan 2022 00:47:56 +0000 (UTC) (envelope-from kevin.bowling@kev009.com) Received: by mail-yb1-xb2b.google.com with SMTP id g81so10436939ybg.10 for ; Wed, 12 Jan 2022 16:47:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kev009.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Tihr5KJX1F8eQHfqFoz7cQOniaOPB12jwQ+7C3iE74U=; b=oDW/HZJjZKwqLwkOAggWScJeILkzCUi9TgvZVv+I7/hXq3PQvUiEFUwqu03x9L6nJg AEs2PoYTd6latUvZZquX+IF0nn49fgigyamgsOBrXg501uv1ECWQrVy50zWWgk4R4gMk 2sj015pjqXmlusOvCO5EvjcfDdwh4zQ5/10Lo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Tihr5KJX1F8eQHfqFoz7cQOniaOPB12jwQ+7C3iE74U=; b=qugyWJh15x3hx2pzEpizKW68hmhrcJu1B9XSRxtgfVDK/hqny1Lp4G61f4w0hh61PU J10mGpxUaKQ4OxoeJ5XoR39VzC2ssAILRAt95N8PpLH7GNfFAMicLSFomqeaSmtdgBUW JncUOQ52+FzZgPa5fWDMK4/tnfb2ig4Qa2JeqlCLodBFz83NsZ/sTBkBLfZdsLvPlufu 94T3r9lMrM9s57pGK2Z3NG2WXvTwePXp/Sn9NzZyHSEnbvCPrx1xvA/dIFbPwZXqiGUf LrCIad6aRhxmhNov9kjadgtWOGCsCcNVlF+aDKGOX8gBXhy+EB0k/V15EJ4gDpTS8uOt nJeQ== X-Gm-Message-State: AOAM533Ddwl9qmiqLNhQXb6cC1qrL/S9Og5JNuj8303ijbIDSJh1Gytx fIQzzLJ5L5RTqJEEi5ThfWTSP6KK2QvYwvZ5EXiUXG6xupM= X-Google-Smtp-Source: ABdhPJwkNpASlDzKFnDhM7Gcr2SVCDqlmy0ppUF98czJ5hYvwn1IPtmjr3IC1SrYOcqbJIK/5sty6AirhJBoysGZH4A= X-Received: by 2002:a25:ad0f:: with SMTP id y15mr3140464ybi.445.1642034875679; Wed, 12 Jan 2022 16:47:55 -0800 (PST) List-Id: Networking and TCP/IP with FreeBSD List-Archive: https://lists.freebsd.org/archives/freebsd-net List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-freebsd-net@freebsd.org MIME-Version: 1.0 References: <27d6f051-3108-a08a-6f1d-670dd8fdb9bb@FreeBSD.org> In-Reply-To: From: Kevin Bowling Date: Wed, 12 Jan 2022 17:47:44 -0700 Message-ID: Subject: Re: iflib_timer() vs ixl_admin_timer() race To: Alexander Motin Cc: Eric Joyner , "Galazka, Krzysztof" , "Joyner, Eric" , freebsd-net@freebsd.org Content-Type: multipart/alternative; boundary="000000000000795f4205d56c0930" X-Rspamd-Queue-Id: 4JZ5Pm3Zxkz3F6n X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-ThisMailContainsUnwantedMimeParts: N --000000000000795f4205d56c0930 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, Jan 12, 2022 at 10:18 AM Alexander Motin 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 > > *Sent:* Wednesday, January 12, 2022 8:22 AM > > *To:* Alexander Motin > > *Cc:* Joyner, Eric ; Galazka, Krzysztof > > > > *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 > > 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 > > > > >> 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


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).=C2=A0 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().=C2=A0 Ouroboros. ;)

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.


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 = so
> I don=E2=80=99t think we can acquire any locks there. IIRC it was adde= d to let
> tools for NVM update interact with FW while interface is down, so mayb= e
> stopping it when interface goes UP would be an option. Let me think th= is
> 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
> <k= rzysztof.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 jus= t that
> 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 i= t
> handles a bunch of other things as well as getting down to calling > ixl_if_update_admin_status() which does the admin queue processing. Bu= t
> 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 ar= e
> 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 tha= t
> 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 mig= ht
> have better thoughts on this.
>
> - Eric
>
> On Tue, Jan 11, 2022 at 10:46 AM Alexander Motin <mav@freebsd.org
> <mailto:mav@fr= eebsd.org>> wrote:
>
>=C2=A0 =C2=A0 =C2=A0Yes.=C2=A0 I've reverted my patch for now to no= t break anything, but all
>=C2=A0 =C2=A0 =C2=A0this
>=C2=A0 =C2=A0 =C2=A0situation does not look right for me too, so I'= d appreciate your look.
>
>=C2=A0 =C2=A0 =C2=A0On 11.01.2022 12:21, Eric Joyner wrote:
>=C2=A0 =C2=A0 =C2=A0 > Hi,
>=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 > I came back from vacation yesterday, but I= 9;ll look into this for you
>=C2=A0 =C2=A0 =C2=A0 > today. It's not a good situation when cha= nging the period of the
>=C2=A0 =C2=A0 =C2=A0iflib
>=C2=A0 =C2=A0 =C2=A0 > timer breaks something in the driver...
>=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 > - Eric
>=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 > On Sun, Jan 9, 2022 at 8:19 PM Alexander Moti= n <mav@freebsd.org<= /a>
>=C2=A0 =C2=A0 =C2=A0<mailto:mav@freebsd.org>
>=C2=A0 =C2=A0 =C2=A0 > <mailto:mav@freebsd.org <mailto:mav@freebsd.org>>> wrote:
>=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0Hi Eric,
>=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0In 90bc1cf65778 I've d= one what looked like a trivial
>=C2=A0 =C2=A0 =C2=A0optimization.=C2=A0 But
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0just after committing it I= 've noticed weird race it created
>=C2=A0 =C2=A0 =C2=A0between
>=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
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0iflib_admin_intr_deferred(= ) every 0.5s, which calls
>=C2=A0 =C2=A0 =C2=A0_task_fn_admin(),
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0which every time stops and= restart txq->ift_timer callout (AKA
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0iflib_timer()), which actu= ally has exactly the same period of
>=C2=A0 =C2=A0 =C2=A00.5s.=C2=A0 It
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0seems before my change ifl= ib_timer() managed to run once for
>=C2=A0 =C2=A0 =C2=A0all TX
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0queues before being stoppe= d, but after the change due to
>=C2=A0 =C2=A0 =C2=A0additional
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0jitter many of callouts ar= e getting stopped before firing.
>=C2=A0 =C2=A0 =C2=A0 >
>=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
>=C2=A0 =C2=A0 =C2=A0there?
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0That race between two call= outs looks like potential bug to
>=C2=A0 =C2=A0 =C2=A0me, working
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0by some coinncedence, espe= cially considering iflib_timer()
>=C2=A0 =C2=A0 =C2=A0period is
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0tunable.
>=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0Thanks in advance.
>=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0--
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0Alexander Motin
>=C2=A0 =C2=A0 =C2=A0 >
>
>=C2=A0 =C2=A0 =C2=A0--
>=C2=A0 =C2=A0 =C2=A0Alexander Motin
>
> ----------------------------------------------------------------------= --
> *Intel Technology Poland sp. z o.o.
> * ul. S=C5=82owackiego 173 | 80-298 Gda=C5=84sk | S=C4=85d Rejonowy Gd= a=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 przypadk= owego
> otrzymania tej wiadomo=C5=9Bci, prosimy o powiadomienie nadawcy oraz t= rwa=C5=82e
> jej usuni=C4=99cie; jakiekolwiek przegl=C4=85danie lub rozpowszechnian= ie 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--