On Tue, Sep 26, 2023 at 4:06 PM Bruce Richardson <bruce.richard...@intel.com> wrote: > > On Tue, Sep 26, 2023 at 01:15:28PM +0100, Zhang, Qi Z wrote: > > > > > > > -----Original Message----- > > > From: Bruce Richardson <bruce.richard...@intel.com> > > > Sent: Tuesday, September 26, 2023 3:49 PM > > > To: He, ShiyangX <shiyangx...@intel.com> > > > Cc: dev@dpdk.org; Zhou, YidingX <yidingx.z...@intel.com>; Wang, Liang- > > > min <liang-min.w...@intel.com>; Su, Simei <simei...@intel.com>; Wu, > > > Wenjun1 <wenjun1...@intel.com>; Zhang, Yuying > > > <yuying.zh...@intel.com>; Xing, Beilei <beilei.x...@intel.com>; Yang, > > > Qiming <qiming.y...@intel.com>; Wu, Jingjing <jingjing...@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...@intel.com> > > > > Signed-off-by: Liang-Min Larry Wang <liang-min.w...@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".
I did not read the patch. You mention that rte_eth_dev_reset will fail now when this (horrific) devargs is passed. Yet, will the driver send a RTE_ETH_EVENT_INTR_RESET event? Additionnally, how does the driver make sure that no application/datapath thread is polling/reconfiguring this hw? -- David Marchand
