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. ;)
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’t 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 this
through.
Thanks,
Krzysiek
*From:* Eric Joyner <e...@freebsd.org>
*Sent:* Wednesday, January 12, 2022 8:22 AM
*To:* Alexander Motin <m...@freebsd.org>
*Cc:* Joyner, Eric <eric.joy...@intel.com>; Galazka, Krzysztof
<krzysztof.gala...@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 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 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 <m...@freebsd.org
<mailto:m...@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 <m...@freebsd.org
<mailto:m...@freebsd.org>
> <mailto:m...@freebsd.org <mailto:m...@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 (AKA
> iflib_timer()), which actually has exactly the same period of
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łowackiego 173 | 80-298 Gdańsk | Sąd Rejonowy Gdańsk Północ | VII
Wydział Gospodarczy Krajowego Rejestru Sądowego - KRS 101882 | NIP
957-07-52-316 | Kapitał zakładowy 200.000 PLN.
Ta wiadomość wraz z załącznikami jest przeznaczona dla określonego
adresata i może zawierać informacje poufne. W razie przypadkowego
otrzymania tej wiadomości, prosimy o powiadomienie nadawcy oraz trwałe
jej usunięcie; jakiekolwiek przeglądanie lub rozpowszechnianie 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
