diff mbox series

[v2] app/procinfo: show all non-owned ports

Message ID 20220525054604.16161-1-subendu@arista.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers
Series [v2] app/procinfo: show all non-owned ports |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-abi-testing success Testing PASS

Commit Message

Subendu Santra May 25, 2022, 5:46 a.m. UTC 
  Show all non-owned ports when no port mask is specified

show-port option without the mask option, displays only the last
non-owned port. Show all the non-owned ports instead.

Fixes: 1dd6cffb6571 ("app/procinfo: provide way to request info on owned
ports")
Cc: stephen@networkplumber.org

Signed-off-by: Subendu Santra <subendu@arista.com>
---
 app/proc-info/main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Stephen Hemminger May 25, 2022, 6:04 a.m. UTC | #1 
On Tue, 24 May 2022 22:46:05 -0700
Subendu Santra <subendu@arista.com> wrote:

> Show all non-owned ports when no port mask is specified
> 
> show-port option without the mask option, displays only the last
> non-owned port. Show all the non-owned ports instead.
> 
> Fixes: 1dd6cffb6571 ("app/procinfo: provide way to request info on owned
> ports")
> Cc: stephen@networkplumber.org
> 
> Signed-off-by: Subendu Santra <subendu@arista.com>
> ---
>  app/proc-info/main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/app/proc-info/main.c b/app/proc-info/main.c
> index 56070a3317..2be24b584e 100644
> --- a/app/proc-info/main.c
> +++ b/app/proc-info/main.c
> @@ -1504,10 +1504,10 @@ main(int argc, char **argv)
>  	if (nb_ports == 0)
>  		rte_exit(EXIT_FAILURE, "No Ethernet ports - bye\n");
>  
> -	/* If no port mask was specified, then show non-owned ports */
> +	/* If no port mask was specified, then show all non-owned ports */
>  	if (enabled_port_mask == 0) {
>  		RTE_ETH_FOREACH_DEV(i)
> -			enabled_port_mask = 1ul << i;
> +			enabled_port_mask |= (1ul << i);

Ok, looks good. parens on that line are unnecessary

Note: this still will have issues with >32 ports on 32 bit platforms.
But other tools probably have same problem.

Acked-by: Stephen Hemminger <stephen@networkplumber.org>
Morten Brørup May 25, 2022, 7:14 a.m. UTC | #2 
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Wednesday, 25 May 2022 08.04
> 
> On Tue, 24 May 2022 22:46:05 -0700
> Subendu Santra <subendu@arista.com> wrote:
> 
> > Show all non-owned ports when no port mask is specified
> >
> > show-port option without the mask option, displays only the last
> > non-owned port. Show all the non-owned ports instead.
> >
> > Fixes: 1dd6cffb6571 ("app/procinfo: provide way to request info on
> owned
> > ports")
> > Cc: stephen@networkplumber.org
> >
> > Signed-off-by: Subendu Santra <subendu@arista.com>
> > ---
> >  app/proc-info/main.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/app/proc-info/main.c b/app/proc-info/main.c
> > index 56070a3317..2be24b584e 100644
> > --- a/app/proc-info/main.c
> > +++ b/app/proc-info/main.c
> > @@ -1504,10 +1504,10 @@ main(int argc, char **argv)
> >  	if (nb_ports == 0)
> >  		rte_exit(EXIT_FAILURE, "No Ethernet ports - bye\n");
> >
> > -	/* If no port mask was specified, then show non-owned ports */
> > +	/* If no port mask was specified, then show all non-owned ports
> */
> >  	if (enabled_port_mask == 0) {
> >  		RTE_ETH_FOREACH_DEV(i)
> > -			enabled_port_mask = 1ul << i;
> > +			enabled_port_mask |= (1ul << i);
> 
> Ok, looks good. parens on that line are unnecessary
> 
> Note: this still will have issues with >32 ports on 32 bit platforms.

The default max_ethports value in meson_options.txt is 32, so the probability is low.

> But other tools probably have same problem.

It was decided many years ago to extend the port_id type from uint8_t to uint16_t, mainly to support a high number of virtual ports. So it is not good that the applications have not been updated accordingly.

However, as Stephen also mentions, this is not unique to this tool, so we'll just ignore it.

> 
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>

Acked-by: Morten Brørup <mb@smartsharesystems.com>
Subendu Santra June 6, 2022, 2:20 p.m. UTC | #3 
Hi Thomas,

How does this patch look? Is there anything that needs to be done from my side?

Regards,
Subendu.



On Wed, May 25, 2022 at 12:44 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Wednesday, 25 May 2022 08.04
> >
> > On Tue, 24 May 2022 22:46:05 -0700
> > Subendu Santra <subendu@arista.com> wrote:
> >
> > > Show all non-owned ports when no port mask is specified
> > >
> > > show-port option without the mask option, displays only the last
> > > non-owned port. Show all the non-owned ports instead.
> > >
> > > Fixes: 1dd6cffb6571 ("app/procinfo: provide way to request info on
> > owned
> > > ports")
> > > Cc: stephen@networkplumber.org
> > >
> > > Signed-off-by: Subendu Santra <subendu@arista.com>
> > > ---
> > >  app/proc-info/main.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/app/proc-info/main.c b/app/proc-info/main.c
> > > index 56070a3317..2be24b584e 100644
> > > --- a/app/proc-info/main.c
> > > +++ b/app/proc-info/main.c
> > > @@ -1504,10 +1504,10 @@ main(int argc, char **argv)
> > >     if (nb_ports == 0)
> > >             rte_exit(EXIT_FAILURE, "No Ethernet ports - bye\n");
> > >
> > > -   /* If no port mask was specified, then show non-owned ports */
> > > +   /* If no port mask was specified, then show all non-owned ports
> > */
> > >     if (enabled_port_mask == 0) {
> > >             RTE_ETH_FOREACH_DEV(i)
> > > -                   enabled_port_mask = 1ul << i;
> > > +                   enabled_port_mask |= (1ul << i);
> >
> > Ok, looks good. parens on that line are unnecessary
> >
> > Note: this still will have issues with >32 ports on 32 bit platforms.
>
> The default max_ethports value in meson_options.txt is 32, so the probability is low.
>
> > But other tools probably have same problem.
>
> It was decided many years ago to extend the port_id type from uint8_t to uint16_t, mainly to support a high number of virtual ports. So it is not good that the applications have not been updated accordingly.
>
> However, as Stephen also mentions, this is not unique to this tool, so we'll just ignore it.
>
> >
> > Acked-by: Stephen Hemminger <stephen@networkplumber.org>
>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>
Thomas Monjalon June 26, 2022, 3:26 p.m. UTC | #4 
25/05/2022 09:14, Morten Brørup:
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Wednesday, 25 May 2022 08.04
> > 
> > On Tue, 24 May 2022 22:46:05 -0700
> > Subendu Santra <subendu@arista.com> wrote:
> > 
> > > Show all non-owned ports when no port mask is specified
> > >
> > > show-port option without the mask option, displays only the last
> > > non-owned port. Show all the non-owned ports instead.

I think it is easier to understand if using past tense
for the previous behaviour. I reword it like this:

    The show-port option, without the mask option,
    was showing only the last non-owned port.
    Show all the non-owned ports instead.


> > > Fixes: 1dd6cffb6571 ("app/procinfo: provide way to request info on
> > owned
> > > ports")
> > > Cc: stephen@networkplumber.org

+   Cc: stable@dpdk.org

> > >
> > > Signed-off-by: Subendu Santra <subendu@arista.com>
> > > ---
> > >  app/proc-info/main.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/app/proc-info/main.c b/app/proc-info/main.c
> > > index 56070a3317..2be24b584e 100644
> > > --- a/app/proc-info/main.c
> > > +++ b/app/proc-info/main.c
> > > @@ -1504,10 +1504,10 @@ main(int argc, char **argv)
> > >  	if (nb_ports == 0)
> > >  		rte_exit(EXIT_FAILURE, "No Ethernet ports - bye\n");
> > >
> > > -	/* If no port mask was specified, then show non-owned ports */
> > > +	/* If no port mask was specified, then show all non-owned ports
> > */
> > >  	if (enabled_port_mask == 0) {
> > >  		RTE_ETH_FOREACH_DEV(i)
> > > -			enabled_port_mask = 1ul << i;
> > > +			enabled_port_mask |= (1ul << i);
> > 
> > Ok, looks good. parens on that line are unnecessary

parens removed

> > 
> > Note: this still will have issues with >32 ports on 32 bit platforms.
> 
> The default max_ethports value in meson_options.txt is 32, so the probability is low.
> 
> > But other tools probably have same problem.
> 
> It was decided many years ago to extend the port_id type from uint8_t to uint16_t, mainly to support a high number of virtual ports. So it is not good that the applications have not been updated accordingly.
> 
> However, as Stephen also mentions, this is not unique to this tool, so we'll just ignore it.
> 
> > 
> > Acked-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> Acked-by: Morten Brørup <mb@smartsharesystems.com>

Applied with small changes, thanks.
diff mbox series

Patch

diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index 56070a3317..2be24b584e 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -1504,10 +1504,10 @@  main(int argc, char **argv)
 	if (nb_ports == 0)
 		rte_exit(EXIT_FAILURE, "No Ethernet ports - bye\n");
 
-	/* If no port mask was specified, then show non-owned ports */
+	/* If no port mask was specified, then show all non-owned ports */
 	if (enabled_port_mask == 0) {
 		RTE_ETH_FOREACH_DEV(i)
-			enabled_port_mask = 1ul << i;
+			enabled_port_mask |= (1ul << i);
 	}
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {