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

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

[Subendu Santra]

Show all non-owned ports when no port mask is specified

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);
 	}
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
-- 
2.28.0

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

[Subendu Santra]

Hi Reshma, Thomas,

What do you think about this patch?

Regards,
Subendu.


On Tue, May 10, 2022 at 6:52 PM Subendu Santra <subendu@arista.com> wrote:
>
> Show all non-owned ports when no port mask is specified
>
> 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);
>         }
>
>         for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> --
> 2.28.0
>

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

[Thomas Monjalon]

23/05/2022 12:53, Subendu Santra:
> Hi Reshma, Thomas,
> 
> What do you think about this patch?

It looks to be a fix, right?
Please add a sentence about the previous behaviour,
explaining what is fixed.

Ideally you should show the commit doing the mistake
with the syntax "Fixes:" as in other commits of this tree.

Thanks


> On Tue, May 10, 2022 at 6:52 PM Subendu Santra <subendu@arista.com> wrote:
> >
> > Show all non-owned ports when no port mask is specified
> >
> > 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);
> >         }
> >
> >         for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> > --
> > 2.28.0
> >
> 

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

[Subendu Santra]

Sure. Will add a sentence about the issue which is getting fixed.
However, I have already added Fixes: 1dd6cffb6571 in the patch.
Is there anything else that needs to be done apart from this w.r.to
"Fixes:" comment?

Regards,
Subendu.


On Mon, May 23, 2022 at 8:12 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 23/05/2022 12:53, Subendu Santra:
> > Hi Reshma, Thomas,
> >
> > What do you think about this patch?
>
> It looks to be a fix, right?
> Please add a sentence about the previous behaviour,
> explaining what is fixed.
>
> Ideally you should show the commit doing the mistake
> with the syntax "Fixes:" as in other commits of this tree.
>
> Thanks
>
>
> > On Tue, May 10, 2022 at 6:52 PM Subendu Santra <subendu@arista.com> wrote:
> > >
> > > Show all non-owned ports when no port mask is specified
> > >
> > > 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);
> > >         }
> > >
> > >         for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> > > --
> > > 2.28.0
> > >
> >
>
>
>
>
>

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

[Thomas Monjalon]

23/05/2022 18:10, Subendu Santra:
> Sure. Will add a sentence about the issue which is getting fixed.
> However, I have already added Fixes: 1dd6cffb6571 in the patch.

Sorry I missed it.

> Is there anything else that needs to be done apart from this w.r.to
> "Fixes:" comment?

No that's fine, thanks.

> 
> Regards,
> Subendu.
> 
> 
> On Mon, May 23, 2022 at 8:12 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > 23/05/2022 12:53, Subendu Santra:
> > > Hi Reshma, Thomas,
> > >
> > > What do you think about this patch?
> >
> > It looks to be a fix, right?
> > Please add a sentence about the previous behaviour,
> > explaining what is fixed.
> >
> > Ideally you should show the commit doing the mistake
> > with the syntax "Fixes:" as in other commits of this tree.
> >
> > Thanks
> >
> >
> > > On Tue, May 10, 2022 at 6:52 PM Subendu Santra <subendu@arista.com> wrote:
> > > >
> > > > Show all non-owned ports when no port mask is specified
> > > >
> > > > 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);
> > > >         }
> > > >
> > > >         for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> > > > --
> > > > 2.28.0
> > > >
> > >
> >
> >
> >
> >
> >
> 

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

[Subendu Santra]

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);
 	}
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
-- 
2.28.0

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

[Subendu Santra]

I have updated the v2 of the patch with the explanation.
Kindly advise if it looks ok.

Regards,
Subendu.


On Wed, May 25, 2022 at 11:22 AM 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);
>         }
>
>         for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> --
> 2.28.0
>

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

[Thomas Monjalon]

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.

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

[Subendu Santra]

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>
>

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

[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.
> >
> > 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>

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

[Stephen Hemminger]

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>

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

[Subendu Santra]

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);
 	}
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
-- 
2.28.0