RE: [PATCH v3] net/iavf: add devargs to enable vf auto-reset

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Tuesday, September 26, 2023 10:06 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: He, ShiyangX <shiyangx.he@intel.com>; 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 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".

Thank you for the suggestion. The commit log has been refined in the dpdk-next-net-intel repo.

Qi

> 
> Regards,
> /Bruce