[dpdk-dev] [PATCH v3 04/10] app/testpmd: convert to new Ethdev Tx offloads API

[Lu, Wenzhuo]

Hi Shahaf,

> -----Original Message-----
> From: Shahaf Shuler [mailto:shahafs at mellanox.com]
> Sent: Tuesday, January 9, 2018 2:48 PM
> To: Lu, Wenzhuo <wenzhuo.lu at intel.com>; Wu, Jingjing
> <jingjing.wu at intel.com>; Yigit, Ferruh <ferruh.yigit at intel.com>
> Cc: dev at dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v3 04/10] app/testpmd: convert to new
> Ethdev Tx offloads API
> 
> Hi,
> 
> Tuesday, January 9, 2018 7:28 AM, Lu, Wenzhuo:
> > Hi Shahaf,
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Shahaf Shuler
> > > Sent: Tuesday, December 26, 2017 5:44 PM
> > > To: Wu, Jingjing <jingjing.wu at intel.com>; Yigit, Ferruh
> > > <ferruh.yigit at intel.com>
> > > Cc: dev at dpdk.org
> > > Subject: [dpdk-dev] [PATCH v3 04/10] app/testpmd: convert to new
> > > Ethdev Tx offloads API
> > >
> > > Ethdev Tx offloads API has changed since:
> > >
> > > commit cba7f53b717d ("ethdev: introduce Tx queue offloads API")
> > >
> > > Convert the application to use the new API.
> > >
> > > This patch mandates the port to be stopped when configure the Tx
> > offloads.
> > > This is because the PMD must be aware to the offloads changes on the
> > > device and queue configuration.
> > >
> > > Signed-off-by: Shahaf Shuler <shahafs at mellanox.com>
> > > ---
> > >  app/test-pmd/cmdline.c | 90
> > > ++++++++++++++++++++++++++++++++++++++++++---
> > >  app/test-pmd/config.c  | 55 ++++++++++++++++++---------  app/test-
> > > pmd/testpmd.c |  3 ++
> > >  3 files changed, 124 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > > d8c73a9b1..58125839a 100644
> > > --- a/app/test-pmd/cmdline.c
> > > +++ b/app/test-pmd/cmdline.c
> > > @@ -3439,7 +3439,14 @@ cmd_tx_vlan_set_parsed(void *parsed_result,
> > {
> > >  	struct cmd_tx_vlan_set_result *res = parsed_result;
> > >
> > > +	if (!port_is_stopped(res->port_id)) {
> > > +		printf("Please stop port %d first\n", res->port_id);
> > > +		return;
> > > +	}
> > > +
> > >  	tx_vlan_set(res->port_id, res->vlan_id);
> > > +
> > > +	cmd_reconfig_device_queue(res->port_id, 1, 1);
> > >  }
> > >
> > I do have some concern about this behavior change, 'port_is_stopped' and
> '
> > cmd_reconfig_device_queue '.
> > 1, seems this behavior change is not necessary for using the new offload
> API.
> > Maybe splitting this patch to 2 is better.
> 
> It is related. Because as part of the offloads API the Tx offloads configuration
> is set on the rte_eth_dev_configure. To configure a device it must be
> stopped in advance.
> 
> Also, per my understanding, the only API which allows offloads configuration
> on the flight are the Rx VLAN offloads.
> As you can see, this API is respected and no need to check the port status in
> it
> 
> > 2, some NICs doesn't need to be stopped or re-configured to make vlan
> > functions enabled.
> 
> Only for the Rx vlan. The Tx vlan insert is not configured on the flight.
> 
> That's why the original code doesn't have this restriction.
> > Maybe figuring out a way to do the restriction in the driver layer is better.
> 
> The original code doesn't have such restriction because testpmd wasn't
> configuring Tx offloads at all. It used it's own Tx offloads enum (which I
> removed on later patches) to indicate which offloads is set. From the device
> perspective all the Tx offloads should be set.
> It did had a way to configure offloads with txqflags parameter, but this
> required queues reconfigurations.
' cmd_tx_vlan_set_parsed' is the function which configures TX vlan insertion. This patch adds the restriction in it.
For example, on ixgbe it does support on the fly configuration. APP doesn't need to stop the device or reconfigure the queues.