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

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

> -----Original Message-----
> From: Singh, Aman Deep <aman.deep.singh@intel.com>
> Sent: Friday, March 10, 2023 9:09 AM
> To: Stephen Hemminger <stephen@networkplumber.org>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>
> Cc: Jangra, Yogesh <yogesh.jangra@intel.com>; 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
> 
> 
> On 3/10/2023 1:52 AM, Stephen Hemminger wrote:
> > On Thu, 9 Mar 2023 17:19:59 +0000
> > "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:
> >
> >>> -----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
> > If the application has to do this then something is wrong with the architecture.
> >
> >
> > You aren't propagating state changes through in a safe manner.
> > Other applications will have same issue.
> >
> > If LAG or virtual devices have similar problems then a generic solution is
> needed.
> 
> At exit, there is call to stop_packet_forwarding(). Shouldn't that stop SoftNic
> from polling of the queues ?

Hi Aman,

Unfortunately not. Calling the stop_packet_forwarding() will not stop the core running the Soft NIC, as Soft NIC is running on a service core, and the service cores are not handled by this function.

Moreover, a Soft NIC device is not taking over the entire service core; the same service core can also run other services e.g. Soft NIC for device A, telemetry, stats, Soft NIC for device B, etc). Therefore, stopping the service core associated with a Soft NIC device will also incorrectly result in stopping all the other services running on the same service core.

Stopping the service for a Soft NIC device is done by the stop() function, which is exactly what we are trying to do with this short patch.

Regards,
Cristian