[v5,1/6] app/testpmd: fix ports list after removing several at once
Checks
Commit Message
From: Wisam Jaddo <wisamm@mellanox.com>
When detaching a port, the full rte_device is removed.
If the rte_device was hosting several ports,
the testpmd list of ports must be updated for multiple removals.
Signed-off-by: Wisam Jaddo <wisamm@mellanox.com>
---
app/test-pmd/testpmd.c | 36 +++++++++++++++++++++++++-----------
1 file changed, 25 insertions(+), 11 deletions(-)
Comments
Hi Wisam,
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, October 18, 2018 2:24 AM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; arybchenko@solarflare.com
> Cc: dev@dpdk.org; ophirmu@mellanox.com; Iremonger, Bernard
> <bernard.iremonger@intel.com>; rahul.lakkireddy@chelsio.com; Wisam Jaddo
> <wisamm@mellanox.com>
> Subject: [PATCH v5 1/6] app/testpmd: fix ports list after removing several at
> once
>
> From: Wisam Jaddo <wisamm@mellanox.com>
>
> When detaching a port, the full rte_device is removed.
> If the rte_device was hosting several ports, the testpmd list of ports must be
> updated for multiple removals.
./devtools/check-git-log.sh -1
Missing 'Fixes' tag:
app/testpmd: fix ports list after removing several at once
> Signed-off-by: Wisam Jaddo <wisamm@mellanox.com>
> ---
> app/test-pmd/testpmd.c | 36 +++++++++++++++++++++++++-----------
> 1 file changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 5dbbf783f..c4109417a 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2186,6 +2186,30 @@ stop_port(portid_t pid)
> printf("Done\n");
> }
>
> +static void
> +remove_unused_fwd_ports(void)
> +{
> + int i;
> + int last_port_idx = nb_ports - 1;
> +
> + for (i = 0; i < last_port_idx + 1; i++) { /* iterate in ports_ids */
Why not use " i <= last_port_index" instead of adding back 1
> + if (rte_eth_devices[ports_ids[i]].state ==
> RTE_ETH_DEV_UNUSED) {
> + /* skip unused ports at the end */
> + while (rte_eth_devices[ports_ids[last_port_idx]].state
> + == RTE_ETH_DEV_UNUSED && i <=
> last_port_idx)
WARNING:LONG_LINE: line over 80 characters
#48: FILE: app/test-pmd/testpmd.c:2199:
+ == RTE_ETH_DEV_UNUSED && i <= last_port_idx)
> + last_port_idx--;
> + if (last_port_idx < i)
> + break;
> + /* overwrite unused port with last valid port */
> + ports_ids[i] = ports_ids[last_port_idx];
> + /* decrease ports count */
> + last_port_idx--;
> + }
> + }
> + nb_ports = rte_eth_dev_count_avail();
> + update_fwd_ports(RTE_MAX_ETHPORTS);
> +}
> +
> void
> close_port(portid_t pid)
> {
> @@ -2315,7 +2339,6 @@ void
> detach_port(portid_t port_id)
> {
> char name[RTE_ETH_NAME_MAX_LEN];
> - uint16_t i;
>
> printf("Detaching a port...\n");
>
> @@ -2332,16 +2355,7 @@ detach_port(portid_t port_id)
> return;
> }
>
> - for (i = 0; i < nb_ports; i++) {
> - if (ports_ids[i] == port_id) {
> - ports_ids[i] = ports_ids[nb_ports-1];
> - ports_ids[nb_ports-1] = 0;
> - break;
> - }
> - }
> - nb_ports = rte_eth_dev_count_avail();
> -
> - update_fwd_ports(RTE_MAX_ETHPORTS);
> + remove_unused_fwd_ports();
>
> printf("Port %u is detached. Now total ports is %d\n",
> port_id, nb_ports);
> --
> 2.19.0
Regards,
Bernard.
18/10/2018 12:40, Iremonger, Bernard:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> >
> > From: Wisam Jaddo <wisamm@mellanox.com>
> >
> > When detaching a port, the full rte_device is removed.
> > If the rte_device was hosting several ports, the testpmd list of ports must be
> > updated for multiple removals.
>
> ./devtools/check-git-log.sh -1
> Missing 'Fixes' tag:
> app/testpmd: fix ports list after removing several at once
I think it is OK.
It is fixing a case which was not tested before.
And we don't really need to backport it.
> > Signed-off-by: Wisam Jaddo <wisamm@mellanox.com>
> > ---
> > app/test-pmd/testpmd.c | 36 +++++++++++++++++++++++++-----------
> > 1 file changed, 25 insertions(+), 11 deletions(-)
> >
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > 5dbbf783f..c4109417a 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -2186,6 +2186,30 @@ stop_port(portid_t pid)
> > printf("Done\n");
> > }
> >
> > +static void
> > +remove_unused_fwd_ports(void)
> > +{
> > + int i;
> > + int last_port_idx = nb_ports - 1;
> > +
> > + for (i = 0; i < last_port_idx + 1; i++) { /* iterate in ports_ids */
>
> Why not use " i <= last_port_index" instead of adding back 1
If Wisam agrees, I can fix it in v6.
> > + if (rte_eth_devices[ports_ids[i]].state ==
> > RTE_ETH_DEV_UNUSED) {
> > + /* skip unused ports at the end */
> > + while (rte_eth_devices[ports_ids[last_port_idx]].state
> > + == RTE_ETH_DEV_UNUSED && i <=
> > last_port_idx)
>
> WARNING:LONG_LINE: line over 80 characters
> #48: FILE: app/test-pmd/testpmd.c:2199:
> + == RTE_ETH_DEV_UNUSED && i <= last_port_idx)
Will fix it.
Anyway, we should reverse the order of this check in order to avoid
accessing ports_ids[-1].
> > + last_port_idx--;
Hi Thomas
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, October 18, 2018 12:29 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>;
> arybchenko@solarflare.com; ophirmu@mellanox.com;
> rahul.lakkireddy@chelsio.com; Wisam Jaddo <wisamm@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH v5 1/6] app/testpmd: fix ports list after
> removing several at once
>
> 18/10/2018 12:40, Iremonger, Bernard:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > >
> > > From: Wisam Jaddo <wisamm@mellanox.com>
> > >
> > > When detaching a port, the full rte_device is removed.
> > > If the rte_device was hosting several ports, the testpmd list of
> > > ports must be updated for multiple removals.
> >
> > ./devtools/check-git-log.sh -1
> > Missing 'Fixes' tag:
> > app/testpmd: fix ports list after removing several at once
>
> I think it is OK.
> It is fixing a case which was not tested before.
> And we don't really need to backport it.
If "fix" is removed from the commit message then the fixes line will not be needed.
However if it is a real fix, then I think fixes line should be added .
>
> > > Signed-off-by: Wisam Jaddo <wisamm@mellanox.com>
<snip>
Regards,
Bernard.
Are we saving cycles by set it to be <= instead of < _ +1?
BRs,
Wisam Jaddo
From: Thomas Monjalon<mailto:thomas@monjalon.net>
Sent: Thursday, October 18, 2018 2:29 PM
To: Iremonger, Bernard<mailto:bernard.iremonger@intel.com>
Cc: dev@dpdk.org<mailto:dev@dpdk.org>; Yigit, Ferruh<mailto:ferruh.yigit@intel.com>; arybchenko@solarflare.com<mailto:arybchenko@solarflare.com>; Ophir Munk<mailto:ophirmu@mellanox.com>; rahul.lakkireddy@chelsio.com<mailto:rahul.lakkireddy@chelsio.com>; Wisam Monther<mailto:wisamm@mellanox.com>
Subject: Re: [dpdk-dev] [PATCH v5 1/6] app/testpmd: fix ports list after removing several at once
18/10/2018 12:40, Iremonger, Bernard:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> >
> > From: Wisam Jaddo <wisamm@mellanox.com>
> >
> > When detaching a port, the full rte_device is removed.
> > If the rte_device was hosting several ports, the testpmd list of ports must be
> > updated for multiple removals.
>
> ./devtools/check-git-log.sh -1
> Missing 'Fixes' tag:
> app/testpmd: fix ports list after removing several at once
I think it is OK.
It is fixing a case which was not tested before.
And we don't really need to backport it.
> > Signed-off-by: Wisam Jaddo <wisamm@mellanox.com>
> > ---
> > app/test-pmd/testpmd.c | 36 +++++++++++++++++++++++++-----------
> > 1 file changed, 25 insertions(+), 11 deletions(-)
> >
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > 5dbbf783f..c4109417a 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -2186,6 +2186,30 @@ stop_port(portid_t pid)
> > printf("Done\n");
> > }
> >
> > +static void
> > +remove_unused_fwd_ports(void)
> > +{
> > + int i;
> > + int last_port_idx = nb_ports - 1;
> > +
> > + for (i = 0; i < last_port_idx + 1; i++) { /* iterate in ports_ids */
>
> Why not use " i <= last_port_index" instead of adding back 1
If Wisam agrees, I can fix it in v6.
> > + if (rte_eth_devices[ports_ids[i]].state ==
> > RTE_ETH_DEV_UNUSED) {
> > + /* skip unused ports at the end */
> > + while (rte_eth_devices[ports_ids[last_port_idx]].state
> > + == RTE_ETH_DEV_UNUSED && i <=
> > last_port_idx)
>
> WARNING:LONG_LINE: line over 80 characters
> #48: FILE: app/test-pmd/testpmd.c:2199:
> + == RTE_ETH_DEV_UNUSED && i <= last_port_idx)
Will fix it.
Anyway, we should reverse the order of this check in order to avoid
accessing ports_ids[-1].
> > + last_port_idx--;
Hi Wisam,
From: Wisam Monther [mailto:wisamm@mellanox.com]
Sent: Thursday, October 18, 2018 12:49 PM
To: Thomas Monjalon <thomas@monjalon.net>; Iremonger, Bernard <bernard.iremonger@intel.com>
Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; arybchenko@solarflare.com; Ophir Munk <ophirmu@mellanox.com>; rahul.lakkireddy@chelsio.com
Subject: RE: [dpdk-dev] [PATCH v5 1/6] app/testpmd: fix ports list after removing several at once
Are we saving cycles by set it to be <= instead of < _ +1?
No idea, just looks cleaner to me.
BRs,
Wisam Jaddo
<snip>
Regards,
Bernard.
18/10/2018 13:41, Iremonger, Bernard:
> Hi Thomas
>
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 18/10/2018 12:40, Iremonger, Bernard:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > >
> > > > From: Wisam Jaddo <wisamm@mellanox.com>
> > > >
> > > > When detaching a port, the full rte_device is removed.
> > > > If the rte_device was hosting several ports, the testpmd list of
> > > > ports must be updated for multiple removals.
> > >
> > > ./devtools/check-git-log.sh -1
> > > Missing 'Fixes' tag:
> > > app/testpmd: fix ports list after removing several at once
> >
> > I think it is OK.
> > It is fixing a case which was not tested before.
> > And we don't really need to backport it.
>
> If "fix" is removed from the commit message then the fixes line will not be needed.
> However if it is a real fix, then I think fixes line should be added .
I know, I am the one implementing this check :-)
If I add a Fixes: line, it will warn about a lack of Cc:stable.
But we don't need to backport this.
That's why I think we can ignore this warning.
Reminder: it is just warning, not a mandatory requirement.
Hi Thomas,
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, October 18, 2018 3:21 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>;
> arybchenko@solarflare.com; ophirmu@mellanox.com;
> rahul.lakkireddy@chelsio.com; Wisam Jaddo <wisamm@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH v5 1/6] app/testpmd: fix ports list after
> removing several at once
>
> 18/10/2018 13:41, Iremonger, Bernard:
> > Hi Thomas
> >
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > 18/10/2018 12:40, Iremonger, Bernard:
> > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > >
> > > > > From: Wisam Jaddo <wisamm@mellanox.com>
> > > > >
> > > > > When detaching a port, the full rte_device is removed.
> > > > > If the rte_device was hosting several ports, the testpmd list of
> > > > > ports must be updated for multiple removals.
> > > >
> > > > ./devtools/check-git-log.sh -1
> > > > Missing 'Fixes' tag:
> > > > app/testpmd: fix ports list after removing several at once
> > >
> > > I think it is OK.
> > > It is fixing a case which was not tested before.
> > > And we don't really need to backport it.
> >
> > If "fix" is removed from the commit message then the fixes line will not be
> needed.
> > However if it is a real fix, then I think fixes line should be added .
>
> I know, I am the one implementing this check :-) If I add a Fixes: line, it will warn
> about a lack of Cc:stable.
> But we don't need to backport this.
> That's why I think we can ignore this warning.
>
> Reminder: it is just warning, not a mandatory requirement.
>
>
As this does not seem to be a normal "fix", how about replacing "fix" with something like "update", then there will be no warning to ignore.
Better not have warnings.
Regards,
Bernard.
18/10/2018 18:42, Iremonger, Bernard:
> Hi Thomas,
>
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 18/10/2018 13:41, Iremonger, Bernard:
> > > Hi Thomas
> > >
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > 18/10/2018 12:40, Iremonger, Bernard:
> > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > >
> > > > > > From: Wisam Jaddo <wisamm@mellanox.com>
> > > > > >
> > > > > > When detaching a port, the full rte_device is removed.
> > > > > > If the rte_device was hosting several ports, the testpmd list of
> > > > > > ports must be updated for multiple removals.
> > > > >
> > > > > ./devtools/check-git-log.sh -1
> > > > > Missing 'Fixes' tag:
> > > > > app/testpmd: fix ports list after removing several at once
> > > >
> > > > I think it is OK.
> > > > It is fixing a case which was not tested before.
> > > > And we don't really need to backport it.
> > >
> > > If "fix" is removed from the commit message then the fixes line will not be
> > needed.
> > > However if it is a real fix, then I think fixes line should be added .
> >
> > I know, I am the one implementing this check :-) If I add a Fixes: line, it will warn
> > about a lack of Cc:stable.
> > But we don't need to backport this.
> > That's why I think we can ignore this warning.
> >
> > Reminder: it is just warning, not a mandatory requirement.
> >
> >
> As this does not seem to be a normal "fix", how about replacing "fix" with something like "update", then there will be no warning to ignore.
> Better not have warnings.
Yes, I thought about it and felt that "update" would not have the same meaning.
If nobody has a better idea, I will replace with "update" instead of "fix".
@@ -2186,6 +2186,30 @@ stop_port(portid_t pid)
printf("Done\n");
}
+static void
+remove_unused_fwd_ports(void)
+{
+ int i;
+ int last_port_idx = nb_ports - 1;
+
+ for (i = 0; i < last_port_idx + 1; i++) { /* iterate in ports_ids */
+ if (rte_eth_devices[ports_ids[i]].state == RTE_ETH_DEV_UNUSED) {
+ /* skip unused ports at the end */
+ while (rte_eth_devices[ports_ids[last_port_idx]].state
+ == RTE_ETH_DEV_UNUSED && i <= last_port_idx)
+ last_port_idx--;
+ if (last_port_idx < i)
+ break;
+ /* overwrite unused port with last valid port */
+ ports_ids[i] = ports_ids[last_port_idx];
+ /* decrease ports count */
+ last_port_idx--;
+ }
+ }
+ nb_ports = rte_eth_dev_count_avail();
+ update_fwd_ports(RTE_MAX_ETHPORTS);
+}
+
void
close_port(portid_t pid)
{
@@ -2315,7 +2339,6 @@ void
detach_port(portid_t port_id)
{
char name[RTE_ETH_NAME_MAX_LEN];
- uint16_t i;
printf("Detaching a port...\n");
@@ -2332,16 +2355,7 @@ detach_port(portid_t port_id)
return;
}
- for (i = 0; i < nb_ports; i++) {
- if (ports_ids[i] == port_id) {
- ports_ids[i] = ports_ids[nb_ports-1];
- ports_ids[nb_ports-1] = 0;
- break;
- }
- }
- nb_ports = rte_eth_dev_count_avail();
-
- update_fwd_ports(RTE_MAX_ETHPORTS);
+ remove_unused_fwd_ports();
printf("Port %u is detached. Now total ports is %d\n",
port_id, nb_ports);