Re: [PATCH] net/iavf: add lock for vf commands

[David Marchand <david.marchand@redhat.com>]

Hello Mike,

On Tue, Dec 20, 2022 at 12:24 AM Mike Pattrick <mkp@redhat.com> wrote:
>
> iavf admin queue commands aren't thread-safe. Bugs surrounding this
> issue can manifest in a variety of ways but frequently pend_cmd is
> over written. Simultaneously executing commands can result in a
> misconfigured device or DPDK sleeping in a thread for 2 second.
>
> Despite this limitation, vf commands may be executed from both
> iavf_dev_alarm_handler() in a control thread and the applications main
> thread. This is trivial to simulate in the testpmd application by
> creating a bond of vf's in active backup mode, and then turning the
> bond off and then on again repeatedly.
>
> Previously [1] was proposed as a potential solution, but this commit
> has been reverted. I propose adding locks until a more complete
> solution is available.

- Hum, this commit is still in the main branch, and I did reproduce
the issue with a E810 nic.
[1] is not enough for the race on pend_cmd (probably some alarm is
still racing with the control thread).

So I think a fix is needed regardless of [1] fate.

- Your patch does make the race disappear, but then, about using a
lock on pend_cmd, I wonder what the point to set it atomically is.

- The same issue probably affects the recently copy/pasted^Wintroduced
driver idpf.


>
> [1] commit cb5c1b91f76f ("net/iavf: add thread for event callbacks")
>
> Fixes: 48de41ca11f0 ("net/avf: enable link status update")
> Fixes: 84108425054a ("net/iavf: support asynchronous virtual channel message")
> Cc: stable@dpdk.org
>
> Signed-off-by: Mike Pattrick <mkp@redhat.com>
> ---

[snip]

Doing a git grep -nC 1 iavf_execute_vf_cmd, I see some unprotected calls.
This is probably a rebase issue if you tested on 21.11.



-- 
David Marchand