DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] app/testpmd: fix forward port ids setting
@ 2017-09-03 13:19 Matan Azrad
  2017-09-04  8:45 ` Gaëtan Rivet
  2017-09-07  7:44 ` [dpdk-dev] " Wu, Jingjing
  0 siblings, 2 replies; 8+ messages in thread
From: Matan Azrad @ 2017-09-03 13:19 UTC (permalink / raw)
  To: Jingjing Wu, Gaetan Rivet, Thomas Monjalon; +Cc: dev, Ori Kam, stable

The corrupted code didn't check the port availability when
it was trying to set the forward port IDs array.
However, when it was counting the number of ports, the availability
was checked by RTE_ETH_FOREACH_DEV iterator.

Hence, even when ETH devices ports were not in ATTACHED state,
the testpmd tried to forward traffic by them and got segmentation
fault at queue access time.

For example:
When EAL command line parameters include two devices, the first
is failsafe with two sub devices and the second is any device,
testpmd gets two devices by the iterator and sets for forwarding
both, the failsafe device and the failsafe first sub device
(instead of the second sub device).
After the first failsafe sub device state was changed to DEFERRED,
testpmd tries to forward traffic through the deferred device
because it didn't check the port availability in setting time.

The fix uses the RTE_ETH_FOREACH_DEV iterator for the forward
port IDs default setting.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 app/test-pmd/testpmd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Hi All
I would like to bring up a discussion to complete this bug
fix.

When user wants to set the list of forwarding ports
by "set portlist" (testpmd command line), the testpmd
application checks the availability of the ports by 
rte_eth_dev_is_valid_port API.
By this way, it gets the DEFERRED port as valid port and 
will try to recieve\send packets via this port.

This scenario will cause the same error as this patch fixes.

Should testpmd allow user to run traffic by DEFERRED port
directly?

If any application wants to check a port availability for device
usage (conf\rxtx), Which API should be used?

According to the patch cb894d99eceb ("ethdev: add deferred intermediate device state"),
DEFERRED ports should be invisible to application,
So maybe the rte_eth_dev_is_valid_port API should be internal
and a new ethdev API should be created for applications.

What do you think?  

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 7d40139..f9bdbf8 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -463,9 +463,10 @@ static void
 set_default_fwd_ports_config(void)
 {
 	portid_t pt_id;
+	int i = 0;
 
-	for (pt_id = 0; pt_id < nb_ports; pt_id++)
-		fwd_ports_ids[pt_id] = pt_id;
+	RTE_ETH_FOREACH_DEV(pt_id)
+		fwd_ports_ids[i++] = pt_id;
 
 	nb_cfg_ports = nb_ports;
 	nb_fwd_ports = nb_ports;
-- 
2.7.4

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] app/testpmd: fix forward port ids setting
  2017-09-03 13:19 [dpdk-dev] [PATCH] app/testpmd: fix forward port ids setting Matan Azrad
@ 2017-09-04  8:45 ` Gaëtan Rivet
  2017-09-04  9:25   ` Matan Azrad
  2017-09-07  7:44 ` [dpdk-dev] " Wu, Jingjing
  1 sibling, 1 reply; 8+ messages in thread
From: Gaëtan Rivet @ 2017-09-04  8:45 UTC (permalink / raw)
  To: Matan Azrad; +Cc: Jingjing Wu, Thomas Monjalon, dev, Ori Kam, stable

Hi Matan,

On Sun, Sep 03, 2017 at 04:19:07PM +0300, Matan Azrad wrote:
> The corrupted code didn't check the port availability when
> it was trying to set the forward port IDs array.
> However, when it was counting the number of ports, the availability
> was checked by RTE_ETH_FOREACH_DEV iterator.
> 
> Hence, even when ETH devices ports were not in ATTACHED state,
> the testpmd tried to forward traffic by them and got segmentation
> fault at queue access time.
> 
> For example:
> When EAL command line parameters include two devices, the first
> is failsafe with two sub devices and the second is any device,
> testpmd gets two devices by the iterator and sets for forwarding
> both, the failsafe device and the failsafe first sub device
> (instead of the second sub device).
> After the first failsafe sub device state was changed to DEFERRED,
> testpmd tries to forward traffic through the deferred device
> because it didn't check the port availability in setting time.
> 
> The fix uses the RTE_ETH_FOREACH_DEV iterator for the forward
> port IDs default setting.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> ---
>  app/test-pmd/testpmd.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> Hi All
> I would like to bring up a discussion to complete this bug
> fix.
> 
> When user wants to set the list of forwarding ports
> by "set portlist" (testpmd command line), the testpmd
> application checks the availability of the ports by 
> rte_eth_dev_is_valid_port API.
> By this way, it gets the DEFERRED port as valid port and 
> will try to recieve\send packets via this port.
> 
> This scenario will cause the same error as this patch fixes.
> 
> Should testpmd allow user to run traffic by DEFERRED port
> directly?
> 
> If any application wants to check a port availability for device
> usage (conf\rxtx), Which API should be used?
> 
> According to the patch cb894d99eceb ("ethdev: add deferred intermediate device state"),
> DEFERRED ports should be invisible to application,
> So maybe the rte_eth_dev_is_valid_port API should be internal
> and a new ethdev API should be created for applications.
> 
> What do you think?  
> 

I think that regardless of the semantic of the DEFERRED state or any
other port handling, the correct assumption is to consider any port
iterated over by RTE_ETH_FOREACH_DEV to be the exact set of devices that
are supposed to be usable. In the event of an API evolution regarding
port states, this macro should remain correct.

So I think your fix is correct, and that the implication of
RTE_ETH_FOREACH_DEV avoiding DEFERRED devices is correct.

> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 7d40139..f9bdbf8 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -463,9 +463,10 @@ static void
>  set_default_fwd_ports_config(void)
>  {
>  	portid_t pt_id;
> +	int i = 0;
>  
> -	for (pt_id = 0; pt_id < nb_ports; pt_id++)
> -		fwd_ports_ids[pt_id] = pt_id;
> +	RTE_ETH_FOREACH_DEV(pt_id)
> +		fwd_ports_ids[i++] = pt_id;
>  
>  	nb_cfg_ports = nb_ports;
>  	nb_fwd_ports = nb_ports;
> -- 
> 2.7.4
> 

-- 
Gaëtan Rivet
6WIND

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] app/testpmd: fix forward port ids setting
  2017-09-04  8:45 ` Gaëtan Rivet
@ 2017-09-04  9:25   ` Matan Azrad
  2017-09-04  9:52     ` Gaëtan Rivet
  0 siblings, 1 reply; 8+ messages in thread
From: Matan Azrad @ 2017-09-04  9:25 UTC (permalink / raw)
  To: Gaëtan Rivet; +Cc: Jingjing Wu, Thomas Monjalon, dev, Ori Kam, stable

Hi

> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Monday, September 4, 2017 11:46 AM
> To: Matan Azrad <matan@mellanox.com>
> Cc: Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; dev@dpdk.org; Ori Kam <orika@mellanox.com>;
> stable@dpdk.org
> Subject: Re: [PATCH] app/testpmd: fix forward port ids setting
> 
> Hi Matan,
> 
> On Sun, Sep 03, 2017 at 04:19:07PM +0300, Matan Azrad wrote:
> > The corrupted code didn't check the port availability when it was
> > trying to set the forward port IDs array.
> > However, when it was counting the number of ports, the availability
> > was checked by RTE_ETH_FOREACH_DEV iterator.
> >
> > Hence, even when ETH devices ports were not in ATTACHED state, the
> > testpmd tried to forward traffic by them and got segmentation fault at
> > queue access time.
> >
> > For example:
> > When EAL command line parameters include two devices, the first is
> > failsafe with two sub devices and the second is any device, testpmd
> > gets two devices by the iterator and sets for forwarding both, the
> > failsafe device and the failsafe first sub device (instead of the
> > second sub device).
> > After the first failsafe sub device state was changed to DEFERRED,
> > testpmd tries to forward traffic through the deferred device because
> > it didn't check the port availability in setting time.
> >
> > The fix uses the RTE_ETH_FOREACH_DEV iterator for the forward port IDs
> > default setting.
> >
> > Fixes: af75078fece3 ("first public release")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > ---
> >  app/test-pmd/testpmd.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > Hi All
> > I would like to bring up a discussion to complete this bug fix.
> >
> > When user wants to set the list of forwarding ports by "set portlist"
> > (testpmd command line), the testpmd application checks the
> > availability of the ports by rte_eth_dev_is_valid_port API.
> > By this way, it gets the DEFERRED port as valid port and will try to
> > recieve\send packets via this port.
> >
> > This scenario will cause the same error as this patch fixes.
> >
> > Should testpmd allow user to run traffic by DEFERRED port directly?
> >
> > If any application wants to check a port availability for device usage
> > (conf\rxtx), Which API should be used?
> >
> > According to the patch cb894d99eceb ("ethdev: add deferred
> > intermediate device state"), DEFERRED ports should be invisible to
> > application, So maybe the rte_eth_dev_is_valid_port API should be
> > internal and a new ethdev API should be created for applications.
> >
> > What do you think?
> >
> 
> I think that regardless of the semantic of the DEFERRED state or any other
> port handling, the correct assumption is to consider any port iterated over by
> RTE_ETH_FOREACH_DEV to be the exact set of devices that are supposed to
> be usable. In the event of an API evolution regarding port states, this macro
> should remain correct.
> 
> So I think your fix is correct, and that the implication of
> RTE_ETH_FOREACH_DEV avoiding DEFERRED devices is correct.
> 

This patch fixes the default forward ports setting (actually when user don't use --portmask param or "set portlist"),
But it don't fix the port validation which PMD does for "set portlist" command.
So, the discussion is how to fix this port validation.
In current code, testpmd uses  rte_eth_dev_is_valid_port which return the DEFERRED device too for forwarding.
Should it use the RTE_ETH_FOREACH_DEV  iterator for one port validation? 
Don't you think we need new API for one port?

> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > 7d40139..f9bdbf8 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -463,9 +463,10 @@ static void
> >  set_default_fwd_ports_config(void)
> >  {
> >  	portid_t pt_id;
> > +	int i = 0;
> >
> > -	for (pt_id = 0; pt_id < nb_ports; pt_id++)
> > -		fwd_ports_ids[pt_id] = pt_id;
> > +	RTE_ETH_FOREACH_DEV(pt_id)
> > +		fwd_ports_ids[i++] = pt_id;
> >
> >  	nb_cfg_ports = nb_ports;
> >  	nb_fwd_ports = nb_ports;
> > --
> > 2.7.4
> >
> 
> --
> Gaëtan Rivet
> 6WIND

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] app/testpmd: fix forward port ids setting
  2017-09-04  9:25   ` Matan Azrad
@ 2017-09-04  9:52     ` Gaëtan Rivet
  2017-09-06 10:21       ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
  0 siblings, 1 reply; 8+ messages in thread
From: Gaëtan Rivet @ 2017-09-04  9:52 UTC (permalink / raw)
  To: Matan Azrad; +Cc: Jingjing Wu, Thomas Monjalon, dev, Ori Kam, stable

On Mon, Sep 04, 2017 at 09:25:04AM +0000, Matan Azrad wrote:
> Hi
> 
> > -----Original Message-----
> > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > Sent: Monday, September 4, 2017 11:46 AM
> > To: Matan Azrad <matan@mellanox.com>
> > Cc: Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon
> > <thomas@monjalon.net>; dev@dpdk.org; Ori Kam <orika@mellanox.com>;
> > stable@dpdk.org
> > Subject: Re: [PATCH] app/testpmd: fix forward port ids setting
> > 
> > Hi Matan,
> > 
> > On Sun, Sep 03, 2017 at 04:19:07PM +0300, Matan Azrad wrote:
> > > The corrupted code didn't check the port availability when it was
> > > trying to set the forward port IDs array.
> > > However, when it was counting the number of ports, the availability
> > > was checked by RTE_ETH_FOREACH_DEV iterator.
> > >
> > > Hence, even when ETH devices ports were not in ATTACHED state, the
> > > testpmd tried to forward traffic by them and got segmentation fault at
> > > queue access time.
> > >
> > > For example:
> > > When EAL command line parameters include two devices, the first is
> > > failsafe with two sub devices and the second is any device, testpmd
> > > gets two devices by the iterator and sets for forwarding both, the
> > > failsafe device and the failsafe first sub device (instead of the
> > > second sub device).
> > > After the first failsafe sub device state was changed to DEFERRED,
> > > testpmd tries to forward traffic through the deferred device because
> > > it didn't check the port availability in setting time.
> > >
> > > The fix uses the RTE_ETH_FOREACH_DEV iterator for the forward port IDs
> > > default setting.
> > >
> > > Fixes: af75078fece3 ("first public release")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > > ---
> > >  app/test-pmd/testpmd.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > Hi All
> > > I would like to bring up a discussion to complete this bug fix.
> > >
> > > When user wants to set the list of forwarding ports by "set portlist"
> > > (testpmd command line), the testpmd application checks the
> > > availability of the ports by rte_eth_dev_is_valid_port API.
> > > By this way, it gets the DEFERRED port as valid port and will try to
> > > recieve\send packets via this port.
> > >
> > > This scenario will cause the same error as this patch fixes.
> > >
> > > Should testpmd allow user to run traffic by DEFERRED port directly?
> > >
> > > If any application wants to check a port availability for device usage
> > > (conf\rxtx), Which API should be used?
> > >
> > > According to the patch cb894d99eceb ("ethdev: add deferred
> > > intermediate device state"), DEFERRED ports should be invisible to
> > > application, So maybe the rte_eth_dev_is_valid_port API should be
> > > internal and a new ethdev API should be created for applications.
> > >
> > > What do you think?
> > >
> > 
> > I think that regardless of the semantic of the DEFERRED state or any other
> > port handling, the correct assumption is to consider any port iterated over by
> > RTE_ETH_FOREACH_DEV to be the exact set of devices that are supposed to
> > be usable. In the event of an API evolution regarding port states, this macro
> > should remain correct.
> > 
> > So I think your fix is correct, and that the implication of
> > RTE_ETH_FOREACH_DEV avoiding DEFERRED devices is correct.
> > 
> 
> This patch fixes the default forward ports setting (actually when user don't use --portmask param or "set portlist"),
> But it don't fix the port validation which PMD does for "set portlist" command.
> So, the discussion is how to fix this port validation.

You could make a static rte_eth_dev_is_valid_port with a different name,
declare both RTE_ETH_VALID_PORT* macros within rte_ethdev.c
and introduce a new rte_eth_dev_is_valid_port which would restrict
devices to those ATTACHED.

I'm not sure this would be interesting for applications. Currently this
check is performed internally by the ether layer, I guess most
applications rely on it. Making the "extended" version (ATTACHED +
DEFERRED) private with the strict one public would probably not change
behaviors, thus it would probably have little to no effect.

So my opinion is "why not, but the risk is adding dead code for no real
benefit".

> In current code, testpmd uses  rte_eth_dev_is_valid_port which return the DEFERRED device too for forwarding.
> Should it use the RTE_ETH_FOREACH_DEV  iterator for one port validation? 
> Don't you think we need new API for one port?
> 
> > > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > > 7d40139..f9bdbf8 100644
> > > --- a/app/test-pmd/testpmd.c
> > > +++ b/app/test-pmd/testpmd.c
> > > @@ -463,9 +463,10 @@ static void
> > >  set_default_fwd_ports_config(void)
> > >  {
> > >  	portid_t pt_id;
> > > +	int i = 0;
> > >
> > > -	for (pt_id = 0; pt_id < nb_ports; pt_id++)
> > > -		fwd_ports_ids[pt_id] = pt_id;
> > > +	RTE_ETH_FOREACH_DEV(pt_id)
> > > +		fwd_ports_ids[i++] = pt_id;
> > >
> > >  	nb_cfg_ports = nb_ports;
> > >  	nb_fwd_ports = nb_ports;
> > > --
> > > 2.7.4
> > >
> > 
> > --
> > Gaëtan Rivet
> > 6WIND

-- 
Gaëtan Rivet
6WIND

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: fix forward port ids setting
  2017-09-04  9:52     ` Gaëtan Rivet
@ 2017-09-06 10:21       ` Thomas Monjalon
  2017-09-06 11:09         ` Matan Azrad
  0 siblings, 1 reply; 8+ messages in thread
From: Thomas Monjalon @ 2017-09-06 10:21 UTC (permalink / raw)
  To: Gaëtan Rivet, Matan Azrad; +Cc: Jingjing Wu, dev, Ori Kam

04/09/2017 11:52, Gaëtan Rivet:
> On Mon, Sep 04, 2017 at 09:25:04AM +0000, Matan Azrad wrote:
> > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > > On Sun, Sep 03, 2017 at 04:19:07PM +0300, Matan Azrad wrote:
> > > > Hi All
> > > > I would like to bring up a discussion to complete this bug fix.
> > > >
> > > > When user wants to set the list of forwarding ports by "set portlist"
> > > > (testpmd command line), the testpmd application checks the
> > > > availability of the ports by rte_eth_dev_is_valid_port API.
> > > > By this way, it gets the DEFERRED port as valid port and will try to
> > > > recieve\send packets via this port.
> > > >
> > > > This scenario will cause the same error as this patch fixes.
> > > >
> > > > Should testpmd allow user to run traffic by DEFERRED port directly?
> > > >
> > > > If any application wants to check a port availability for device usage
> > > > (conf\rxtx), Which API should be used?
> > > >
> > > > According to the patch cb894d99eceb ("ethdev: add deferred
> > > > intermediate device state"), DEFERRED ports should be invisible to
> > > > application, So maybe the rte_eth_dev_is_valid_port API should be
> > > > internal and a new ethdev API should be created for applications.
> > > >
> > > > What do you think?
> > > >
> > > 
> > > I think that regardless of the semantic of the DEFERRED state or any other
> > > port handling, the correct assumption is to consider any port iterated over by
> > > RTE_ETH_FOREACH_DEV to be the exact set of devices that are supposed to
> > > be usable. In the event of an API evolution regarding port states, this macro
> > > should remain correct.
> > > 
> > > So I think your fix is correct, and that the implication of
> > > RTE_ETH_FOREACH_DEV avoiding DEFERRED devices is correct.
> > > 
> > 
> > This patch fixes the default forward ports setting (actually when user don't use --portmask param or "set portlist"),
> > But it don't fix the port validation which PMD does for "set portlist" command.
> > So, the discussion is how to fix this port validation.
> 
> You could make a static rte_eth_dev_is_valid_port with a different name,
> declare both RTE_ETH_VALID_PORT* macros within rte_ethdev.c
> and introduce a new rte_eth_dev_is_valid_port which would restrict
> devices to those ATTACHED.
> 
> I'm not sure this would be interesting for applications. Currently this
> check is performed internally by the ether layer, I guess most
> applications rely on it. Making the "extended" version (ATTACHED +
> DEFERRED) private with the strict one public would probably not change
> behaviors, thus it would probably have little to no effect.
> 
> So my opinion is "why not, but the risk is adding dead code for no real
> benefit".
> 
> > In current code, testpmd uses  rte_eth_dev_is_valid_port which return the DEFERRED device too for forwarding.
> > Should it use the RTE_ETH_FOREACH_DEV  iterator for one port validation? 
> > Don't you think we need new API for one port?

Please, let's continue this ethdev discussion in a separate thread.
I've started a new one:
	http://dpdk.org/ml/archives/dev/2017-September/074656.html

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: fix forward port ids setting
  2017-09-06 10:21       ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
@ 2017-09-06 11:09         ` Matan Azrad
  0 siblings, 0 replies; 8+ messages in thread
From: Matan Azrad @ 2017-09-06 11:09 UTC (permalink / raw)
  To: Jingjing Wu; +Cc: dev, Ori Kam, Thomas Monjalon, Gaëtan Rivet

Hi Jinging,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, September 6, 2017 1:21 PM
> To: Gaëtan Rivet <gaetan.rivet@6wind.com>; Matan Azrad
> <matan@mellanox.com>
> Cc: Jingjing Wu <jingjing.wu@intel.com>; dev@dpdk.org; Ori Kam
> <orika@mellanox.com>
> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix forward port ids setting
> 
> 04/09/2017 11:52, Gaëtan Rivet:
> > On Mon, Sep 04, 2017 at 09:25:04AM +0000, Matan Azrad wrote:
> > > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > > > On Sun, Sep 03, 2017 at 04:19:07PM +0300, Matan Azrad wrote:
> > > > > Hi All
> > > > > I would like to bring up a discussion to complete this bug fix.
> > > > >
> > > > > When user wants to set the list of forwarding ports by "set portlist"
> > > > > (testpmd command line), the testpmd application checks the
> > > > > availability of the ports by rte_eth_dev_is_valid_port API.
> > > > > By this way, it gets the DEFERRED port as valid port and will
> > > > > try to recieve\send packets via this port.
> > > > >
> > > > > This scenario will cause the same error as this patch fixes.
> > > > >
> > > > > Should testpmd allow user to run traffic by DEFERRED port directly?
> > > > >
> > > > > If any application wants to check a port availability for device
> > > > > usage (conf\rxtx), Which API should be used?
> > > > >
> > > > > According to the patch cb894d99eceb ("ethdev: add deferred
> > > > > intermediate device state"), DEFERRED ports should be invisible
> > > > > to application, So maybe the rte_eth_dev_is_valid_port API
> > > > > should be internal and a new ethdev API should be created for
> applications.
> > > > >
> > > > > What do you think?
> > > > >
> > > >
> > > > I think that regardless of the semantic of the DEFERRED state or
> > > > any other port handling, the correct assumption is to consider any
> > > > port iterated over by RTE_ETH_FOREACH_DEV to be the exact set of
> > > > devices that are supposed to be usable. In the event of an API
> > > > evolution regarding port states, this macro should remain correct.
> > > >
> > > > So I think your fix is correct, and that the implication of
> > > > RTE_ETH_FOREACH_DEV avoiding DEFERRED devices is correct.
> > > >
> > >
> > > This patch fixes the default forward ports setting (actually when
> > > user don't use --portmask param or "set portlist"), But it don't fix the port
> validation which PMD does for "set portlist" command.
> > > So, the discussion is how to fix this port validation.
> >
> > You could make a static rte_eth_dev_is_valid_port with a different
> > name, declare both RTE_ETH_VALID_PORT* macros within rte_ethdev.c
> and
> > introduce a new rte_eth_dev_is_valid_port which would restrict devices
> > to those ATTACHED.
> >
> > I'm not sure this would be interesting for applications. Currently
> > this check is performed internally by the ether layer, I guess most
> > applications rely on it. Making the "extended" version (ATTACHED +
> > DEFERRED) private with the strict one public would probably not change
> > behaviors, thus it would probably have little to no effect.
> >
> > So my opinion is "why not, but the risk is adding dead code for no
> > real benefit".
> >
> > > In current code, testpmd uses  rte_eth_dev_is_valid_port which return
> the DEFERRED device too for forwarding.
> > > Should it use the RTE_ETH_FOREACH_DEV  iterator for one port
> validation?
> > > Don't you think we need new API for one port?
> 
> Please, let's continue this ethdev discussion in a separate thread.
> I've started a new one:
> 	https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F
> %2Fdpdk.org%2Fml%2Farchives%2Fdev%2F2017-
> September%2F074656.html&data=02%7C01%7Corika%40mellanox.com%7C5
> 9017f577e004c8be80c08d4f51104ec%7Ca652971c7d2e4d9ba6a4d149256f461b
> %7C0%7C0%7C636402900840946032&sdata=lPFh1ro1cJTyiYYC7KQtRm7CQ8M
> rkct7i6%2BUBW1HEsM%3D&reserved=0

I think you can acknowledge this fix for the default forward port IDs setting (this patch fixes it).
I will send fix in a separated patch to the  "set portlist" port validation after ethdev discussion will be done.

Regards,
Matan Azrad

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] app/testpmd: fix forward port ids setting
  2017-09-03 13:19 [dpdk-dev] [PATCH] app/testpmd: fix forward port ids setting Matan Azrad
  2017-09-04  8:45 ` Gaëtan Rivet
@ 2017-09-07  7:44 ` Wu, Jingjing
  2017-10-09  5:13   ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
  1 sibling, 1 reply; 8+ messages in thread
From: Wu, Jingjing @ 2017-09-07  7:44 UTC (permalink / raw)
  To: Matan Azrad, Gaetan Rivet, Thomas Monjalon; +Cc: dev, Ori Kam, stable



> -----Original Message-----
> From: Matan Azrad [mailto:matan@mellanox.com]
> Sent: Sunday, September 3, 2017 9:19 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>; Gaetan Rivet <gaetan.rivet@6wind.com>;
> Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>; stable@dpdk.org
> Subject: [PATCH] app/testpmd: fix forward port ids setting
> 
> The corrupted code didn't check the port availability when
> it was trying to set the forward port IDs array.
> However, when it was counting the number of ports, the availability
> was checked by RTE_ETH_FOREACH_DEV iterator.
> 
> Hence, even when ETH devices ports were not in ATTACHED state,
> the testpmd tried to forward traffic by them and got segmentation
> fault at queue access time.
> 
> For example:
> When EAL command line parameters include two devices, the first
> is failsafe with two sub devices and the second is any device,
> testpmd gets two devices by the iterator and sets for forwarding
> both, the failsafe device and the failsafe first sub device
> (instead of the second sub device).
> After the first failsafe sub device state was changed to DEFERRED,
> testpmd tries to forward traffic through the deferred device
> because it didn't check the port availability in setting time.
> 
> The fix uses the RTE_ETH_FOREACH_DEV iterator for the forward
> port IDs default setting.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>

>From the view of testpmd, the fix is good.

Acked-by: Jingjing Wu <jingjing.wu@intel.com>

Thanks
Jingjing

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: fix forward port ids setting
  2017-09-07  7:44 ` [dpdk-dev] " Wu, Jingjing
@ 2017-10-09  5:13   ` Ferruh Yigit
  0 siblings, 0 replies; 8+ messages in thread
From: Ferruh Yigit @ 2017-10-09  5:13 UTC (permalink / raw)
  To: Wu, Jingjing, Matan Azrad, Gaetan Rivet, Thomas Monjalon
  Cc: dev, Ori Kam, stable

On 9/7/2017 8:44 AM, Wu, Jingjing wrote:
> 
> 
>> -----Original Message-----
>> From: Matan Azrad [mailto:matan@mellanox.com]
>> Sent: Sunday, September 3, 2017 9:19 PM
>> To: Wu, Jingjing <jingjing.wu@intel.com>; Gaetan Rivet <gaetan.rivet@6wind.com>;
>> Thomas Monjalon <thomas@monjalon.net>
>> Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>; stable@dpdk.org
>> Subject: [PATCH] app/testpmd: fix forward port ids setting
>>
>> The corrupted code didn't check the port availability when
>> it was trying to set the forward port IDs array.
>> However, when it was counting the number of ports, the availability
>> was checked by RTE_ETH_FOREACH_DEV iterator.
>>
>> Hence, even when ETH devices ports were not in ATTACHED state,
>> the testpmd tried to forward traffic by them and got segmentation
>> fault at queue access time.
>>
>> For example:
>> When EAL command line parameters include two devices, the first
>> is failsafe with two sub devices and the second is any device,
>> testpmd gets two devices by the iterator and sets for forwarding
>> both, the failsafe device and the failsafe first sub device
>> (instead of the second sub device).
>> After the first failsafe sub device state was changed to DEFERRED,
>> testpmd tries to forward traffic through the deferred device
>> because it didn't check the port availability in setting time.
>>
>> The fix uses the RTE_ETH_FOREACH_DEV iterator for the forward
>> port IDs default setting.
>>
>> Fixes: af75078fece3 ("first public release")

I think this has been broken by introducing DEFERRED state without
fixing iterators, it wasn't broken previously, so updating fixes line as:

Fixes: cb894d99eceb ("ethdev: add deferred intermediate device state")

>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Matan Azrad <matan@mellanox.com>

> Acked-by: Jingjing Wu <jingjing.wu@intel.com>

Applied to dpdk-next-net/master, thanks.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2017-10-09  5:13 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-03 13:19 [dpdk-dev] [PATCH] app/testpmd: fix forward port ids setting Matan Azrad
2017-09-04  8:45 ` Gaëtan Rivet
2017-09-04  9:25   ` Matan Azrad
2017-09-04  9:52     ` Gaëtan Rivet
2017-09-06 10:21       ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
2017-09-06 11:09         ` Matan Azrad
2017-09-07  7:44 ` [dpdk-dev] " Wu, Jingjing
2017-10-09  5:13   ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).