patches for DPDK stable branches
 help / color / mirror / Atom feed
From: David Marchand <david.marchand@redhat.com>
To: Mike Pattrick <mkp@redhat.com>
Cc: dev@dpdk.org, yidingx.zhou@intel.com, ktraynor@redhat.com,
	stable@dpdk.org, Qi Zhang <qi.z.zhang@intel.com>,
	Ferruh Yigit <ferruh.yigit@amd.com>,
	 Thomas Monjalon <thomas@monjalon.net>
Subject: Re: [PATCH] net/iavf: add lock for vf commands
Date: Tue, 20 Dec 2022 16:02:05 +0100	[thread overview]
Message-ID: <CAJFAV8z=b2JOLf+RU41KE4_=TcktWvvM0aB43TMtusop5p7KVw@mail.gmail.com> (raw)
In-Reply-To: <20221219232332.190466-1-mkp@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


  reply	other threads:[~2022-12-20 15:02 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-19 23:23 Mike Pattrick
2022-12-20 15:02 ` David Marchand [this message]
2022-12-20 15:54   ` [PATCH v2] " Mike Pattrick
2022-12-26  6:01     ` Zhang, Qi Z
2022-12-28 22:28     ` [PATCH v3] " Mike Pattrick
2022-12-28 23:00     ` [PATCH v4] " Mike Pattrick
2023-01-13 13:35       ` David Marchand
2023-01-17  1:20       ` Zhang, Qi Z

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAJFAV8z=b2JOLf+RU41KE4_=TcktWvvM0aB43TMtusop5p7KVw@mail.gmail.com' \
    --to=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@amd.com \
    --cc=ktraynor@redhat.com \
    --cc=mkp@redhat.com \
    --cc=qi.z.zhang@intel.com \
    --cc=stable@dpdk.org \
    --cc=thomas@monjalon.net \
    --cc=yidingx.zhou@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).