From: Bruce Richardson <bruce.richardson@intel.com>
To: "Zhang, Qi Z" <qi.z.zhang@intel.com>
Cc: "He, ShiyangX" <shiyangx.he@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>,
"Zhou, YidingX" <yidingx.zhou@intel.com>,
"Wang, Liang-min" <liang-min.wang@intel.com>,
"Su, Simei" <simei.su@intel.com>,
"Wu, Wenjun1" <wenjun1.wu@intel.com>,
"Zhang, Yuying" <yuying.zhang@intel.com>,
"Xing, Beilei" <beilei.xing@intel.com>,
"Yang, Qiming" <qiming.yang@intel.com>,
"Wu, Jingjing" <jingjing.wu@intel.com>
Subject: Re: [PATCH v3] net/iavf: add devargs to enable vf auto-reset
Date: Tue, 26 Sep 2023 15:05:47 +0100 [thread overview]
Message-ID: <ZRLlO4k85qHwRyfD@bricha3-MOBL.ger.corp.intel.com> (raw)
In-Reply-To: <DM4PR11MB5994625211E14A7DC8660287D7C3A@DM4PR11MB5994.namprd11.prod.outlook.com>
On Tue, Sep 26, 2023 at 01:15:28PM +0100, Zhang, Qi Z wrote:
>
>
> > -----Original Message-----
> > From: Bruce Richardson <bruce.richardson@intel.com>
> > Sent: Tuesday, September 26, 2023 3:49 PM
> > To: He, ShiyangX <shiyangx.he@intel.com>
> > Cc: dev@dpdk.org; Zhou, YidingX <yidingx.zhou@intel.com>; Wang, Liang-
> > min <liang-min.wang@intel.com>; Su, Simei <simei.su@intel.com>; Wu,
> > Wenjun1 <wenjun1.wu@intel.com>; Zhang, Yuying
> > <yuying.zhang@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Yang,
> > Qiming <qiming.yang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> > Subject: Re: [PATCH v3] net/iavf: add devargs to enable vf auto-reset
> >
> > On Fri, Sep 15, 2023 at 01:02:49PM +0000, Shiyang He wrote:
> > > Originally, the iavf PMD does not perform special actions when it
> > > receives a PF-to-VF reset event, resulting in vf being offline and
> > > unavailable.
> > >
> > > This patch enables vf auto-reset by setting 'watchdog_period' devargs
> > > to true. The iavf PMD will perform an automatic reset to bring the vf
> > > back online when it receives a PF-to-VF event.
> > >
> > > v2: handling reset by event handler
> > > v3: change reset process
> > >
> > > Signed-off-by: Shiyang He <shiyangx.he@intel.com>
> > > Signed-off-by: Liang-Min Larry Wang <liang-min.wang@intel.com>
> > > ---
> > > doc/guides/nics/intel_vf.rst | 3 +
> > > doc/guides/rel_notes/release_23_11.rst | 3 +
> > > drivers/net/iavf/iavf.h | 7 +++
> > > drivers/net/iavf/iavf_ethdev.c | 86 +++++++++++++++++++++++---
> > > drivers/net/iavf/iavf_rxtx.c | 52 ++++++++++------
> > > drivers/net/iavf/iavf_vchnl.c | 11 +++-
> > > 6 files changed, 135 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/doc/guides/nics/intel_vf.rst
> > > b/doc/guides/nics/intel_vf.rst index d365dbc185..c0acd2a7f5 100644
> > > --- a/doc/guides/nics/intel_vf.rst
> > > +++ b/doc/guides/nics/intel_vf.rst
> > > @@ -101,6 +101,9 @@ For more detail on SR-IOV, please refer to the
> > following documents:
> > > Set ``devargs`` parameter ``watchdog_period`` to adjust the watchdog
> > period in microseconds, or set it to 0 to disable the watchdog,
> > > for example, ``-a 18:01.0,watchdog_period=5000`` or ``-a
> > 18:01.0,watchdog_period=0``.
> > >
> > > + Enable vf auto-reset by setting the ``devargs`` parameter like ``-a
> > 18:01.0,enable_auto_reset=1`` when IAVF is backed
> > > + by an Intel(r) E810 device or an Intel(r) 700 Series Ethernet device.
> > > +
> >
> > Why do we need a devargs for this? If the VF is unavailable - as you mention
> > in the commit log above - should this behaviour not always be the case
> > without the user having to ask?
>
> Ideally it does not need, but with below considerations:
>
> 1. Not break existing scenario, which still assume some application will call dev_reset /dev_configure/ queue_setup / ... after receive RTE_ETH_EVENT_INTR_RESET event to recover the VF manually, the devargs make sure application be aware of this new feature and will not call rte_eth_dev_reset which will fail now.
>
> 2. intent to ensure a smoother transition, in case some corner case issues evaded our validation, keeping this devargs provides us with the flexibility to remove it once we determine that the implementation is stable enough.
>
Thanks for the clear explanation.
One small suggestion: in the commit log, at the end of the first paragraph
change "resulting in the VF being offline and unavailable" to "... offline
and unavailable until the application resets the device on receipt of the
RTE_ETH_EVENT_INTR_RESET event". Similarly at the end of the second
paragraph you could add "This change removes the need for the application
to handle the reset event, as it is transparently handled inside the
driver".
Regards,
/Bruce
next prev parent reply other threads:[~2023-09-26 14:06 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-01 9:48 [PATCH] " Shiyang He
2023-08-11 9:40 ` [PATCH v2] " Shiyang He
2023-09-15 13:02 ` [PATCH v3] " Shiyang He
2023-09-26 7:48 ` Bruce Richardson
2023-09-26 12:15 ` Zhang, Qi Z
2023-09-26 14:05 ` Bruce Richardson [this message]
2023-09-26 23:41 ` Zhang, Qi Z
2023-09-27 6:39 ` David Marchand
2023-09-26 11:31 ` [PATCH v4] " Shiyang He
2023-09-26 4:51 ` Zhang, Qi Z
2023-09-26 12:29 ` [PATCH v5] " Shiyang He
2023-09-26 12:31 ` Shiyang He
2023-09-26 12:38 ` Shiyang He
2023-09-26 5:33 ` 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=ZRLlO4k85qHwRyfD@bricha3-MOBL.ger.corp.intel.com \
--to=bruce.richardson@intel.com \
--cc=beilei.xing@intel.com \
--cc=dev@dpdk.org \
--cc=jingjing.wu@intel.com \
--cc=liang-min.wang@intel.com \
--cc=qi.z.zhang@intel.com \
--cc=qiming.yang@intel.com \
--cc=shiyangx.he@intel.com \
--cc=simei.su@intel.com \
--cc=wenjun1.wu@intel.com \
--cc=yidingx.zhou@intel.com \
--cc=yuying.zhang@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).