RE: [PATCH] app/testpmd: fix closing softnic port before ethdev ports

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>]

> -----Original Message-----
> From: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Sent: Thursday, March 9, 2023 5:20 PM
> To: Stephen Hemminger <stephen@networkplumber.org>; Jangra, Yogesh
> <yogesh.jangra@intel.com>
> Cc: dev@dpdk.org; R, Kamalakannan <kamalakannan.r@intel.com>; Suresh
> Narayane, Harshad <harshad.suresh.narayane@intel.com>
> Subject: RE: [PATCH] app/testpmd: fix closing softnic port before ethdev ports
> 
> 
> 
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Thursday, March 9, 2023 4:31 PM
> > To: Jangra, Yogesh <yogesh.jangra@intel.com>
> > Cc: dev@dpdk.org; Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; R,
> > Kamalakannan <kamalakannan.r@intel.com>; Suresh Narayane, Harshad
> > <harshad.suresh.narayane@intel.com>
> > Subject: Re: [PATCH] app/testpmd: fix closing softnic port before ethdev ports
> >
> > On Thu,  9 Mar 2023 14:42:49 +0000
> > Yogesh Jangra <yogesh.jangra@intel.com> wrote:
> >
> > > +		/*
> > > +		 * SoftNIC runs on the sevice core, it uses the resources from
> > > +		 * the testpmd application. When we run quit command, the
> > testpmd
> > > +		 * application stops ethdev ports first, SoftNIC will try to
> > > +		 * access the port and sometimes that result in segmentation
> > > +		 * error. So first closing the SoftNIC port.
> > > +		 */
> > > +		RTE_ETH_FOREACH_DEV(pt_id) {
> > > +			if (!strcmp(ports[pt_id].dev_info.driver_name,
> > "net_softnic")) {
> > > +				stop_port(pt_id);
> > > +				close_port(pt_id);
> > > +			}
> > > +		}
> > > +
> >
> > NAK
> > No driver specific hacks please.
> >
> > Instead fix the driver design or bug please.
> 
> Hi Stephen,
> 
> This is not a Soft NIC driver-specific hack, this is required for working around
> some of the ethdev drivers that don't implement the stop() API correctly and
> free up the device queues or some other internal resources on stop() instead of
> close().
> 
> The Soft NIC is a meta-device that sits on top of other "physical" ethdev devices,
> so when the Soft NIC device continues to poll the queues of those physical
> devices after their queues have been freed, the Soft NIC will get a segfault. This
> fix is required to protect against this sort of incorrect driver behavior by simply
> stopping the Soft NIC devices first.
> 
> We already have several driver specific branches in the test-pmd for e.g. LAG or
> virtual devices; IMO this small change falls in the same category and it should
> get accepted.
> 
> Please let us know if this makes sense to you?
> 
> Regards,
> Cristian

Adding Aman and Yuying, the test-pmd maintainers, to this conversation.