[PATCH] sched: fix for tc_ov_enable flag position in subport structure.

[PATCH] sched: fix for tc_ov_enable flag position in subport structure.

[Megha Ajmera]

Current position of "tv_ov_enable" variable in
rte_sched_subport structure makes the "memory" variable unused.

Fixes: f5e60154ade ("sched: enable traffic class oversubscription conditionally")
CC: marcinx.danilewicz@intel.com
Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
---
 lib/sched/rte_sched.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/sched/rte_sched.c b/lib/sched/rte_sched.c
index c91697131d..eaecd7ceb4 100644
--- a/lib/sched/rte_sched.c
+++ b/lib/sched/rte_sched.c
@@ -202,6 +202,9 @@ struct rte_sched_subport {
 	uint32_t qsize_add[RTE_SCHED_QUEUES_PER_PIPE];
 	uint32_t qsize_sum;
 
+	/* TC oversubscription activation */
+	int tc_ov_enabled;
+
 	struct rte_sched_pipe *pipe;
 	struct rte_sched_queue *queue;
 	struct rte_sched_queue_extra *queue_extra;
@@ -210,8 +213,6 @@ struct rte_sched_subport {
 	struct rte_mbuf **queue_array;
 	uint8_t memory[0] __rte_cache_aligned;
 
-	/* TC oversubscription activation */
-	int tc_ov_enabled;
 } __rte_cache_aligned;
 
 struct rte_sched_port {
-- 
2.25.1

RE: [PATCH] sched: fix for tc_ov_enable flag position in subport structure.

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Ajmera, Megha <megha.ajmera@intel.com>
> Sent: Monday, January 9, 2023 2:55 PM
> To: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>; Dumitrescu,
> Cristian <cristian.dumitrescu@intel.com>
> Cc: stable@dpdk.org; marcinx.danilewicz@intel.com
> Subject: [PATCH] sched: fix for tc_ov_enable flag position in subport structure.
> 
> Current position of "tv_ov_enable" variable in
> rte_sched_subport structure makes the "memory" variable unused.
> 
> Fixes: f5e60154ade ("sched: enable traffic class oversubscription conditionally")
> CC: marcinx.danilewicz@intel.com
> Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
> ---
>  lib/sched/rte_sched.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/sched/rte_sched.c b/lib/sched/rte_sched.c
> index c91697131d..eaecd7ceb4 100644
> --- a/lib/sched/rte_sched.c
> +++ b/lib/sched/rte_sched.c
> @@ -202,6 +202,9 @@ struct rte_sched_subport {

Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

Re: [PATCH] sched: fix for tc_ov_enable flag position in subport structure.

[David Marchand]

On Mon, Jan 9, 2023 at 3:59 PM Megha Ajmera <megha.ajmera@intel.com> wrote:
>
> Current position of "tv_ov_enable" variable in

tc_ov_enabled*


> rte_sched_subport structure makes the "memory" variable unused.

I did not enter the beast... but my understanding is that some object
internal to rte_sched_subport currently shares location with this
tc_ov_enabled field.
So please find a better title and describe the impact.


>
> Fixes: f5e60154ade ("sched: enable traffic class oversubscription conditionally")
> CC: marcinx.danilewicz@intel.com

This is stable@dpdk.org material, isn't it?


> Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
> ---
>  lib/sched/rte_sched.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/lib/sched/rte_sched.c b/lib/sched/rte_sched.c
> index c91697131d..eaecd7ceb4 100644
> --- a/lib/sched/rte_sched.c
> +++ b/lib/sched/rte_sched.c
> @@ -202,6 +202,9 @@ struct rte_sched_subport {
>         uint32_t qsize_add[RTE_SCHED_QUEUES_PER_PIPE];
>         uint32_t qsize_sum;
>
> +       /* TC oversubscription activation */
> +       int tc_ov_enabled;
> +
>         struct rte_sched_pipe *pipe;
>         struct rte_sched_queue *queue;
>         struct rte_sched_queue_extra *queue_extra;
> @@ -210,8 +213,6 @@ struct rte_sched_subport {
>         struct rte_mbuf **queue_array;
>         uint8_t memory[0] __rte_cache_aligned;
>
> -       /* TC oversubscription activation */
> -       int tc_ov_enabled;
>  } __rte_cache_aligned;
>
>  struct rte_sched_port {



-- 
David Marchand

Re: [PATCH] sched: fix for tc_ov_enable flag position in subport structure.

[Thomas Monjalon]

10/01/2023 12:27, David Marchand:
> On Mon, Jan 9, 2023 at 3:59 PM Megha Ajmera <megha.ajmera@intel.com> wrote:
> >
> > Current position of "tv_ov_enable" variable in
> 
> tc_ov_enabled*
> 
> 
> > rte_sched_subport structure makes the "memory" variable unused.
> 
> I did not enter the beast... but my understanding is that some object
> internal to rte_sched_subport currently shares location with this
> tc_ov_enabled field.
> So please find a better title and describe the impact.
> 
> 
> >
> > Fixes: f5e60154ade ("sched: enable traffic class oversubscription conditionally")
> > CC: marcinx.danilewicz@intel.com
> 
> This is stable@dpdk.org material, isn't it?
> 
> 
> > Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>

Ping
Please update or the patch will be abandoned.
Jasvinder, Cristian, can you help?

RE: [PATCH] sched: fix for tc_ov_enable flag position in subport structure.

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Monday, February 6, 2023 7:43 AM
> To: Ajmera, Megha <megha.ajmera@intel.com>
> Cc: stable@dpdk.org; dev@dpdk.org; Singh, Jasvinder
> <jasvinder.singh@intel.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>; marcinx.danilewicz@intel.com; David
> Marchand <david.marchand@redhat.com>
> Subject: Re: [PATCH] sched: fix for tc_ov_enable flag position in subport
> structure.
> 
> 10/01/2023 12:27, David Marchand:
> > On Mon, Jan 9, 2023 at 3:59 PM Megha Ajmera <megha.ajmera@intel.com>
> wrote:
> > >
> > > Current position of "tv_ov_enable" variable in
> >
> > tc_ov_enabled*
> >
> >
> > > rte_sched_subport structure makes the "memory" variable unused.
> >
> > I did not enter the beast... but my understanding is that some object
> > internal to rte_sched_subport currently shares location with this
> > tc_ov_enabled field.
> > So please find a better title and describe the impact.
> >
> >
> > >
> > > Fixes: f5e60154ade ("sched: enable traffic class oversubscription
> conditionally")
> > > CC: marcinx.danilewicz@intel.com
> >
> > This is stable@dpdk.org material, isn't it?
> >
> >
> > > Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
> 
> Ping
> Please update or the patch will be abandoned.
> Jasvinder, Cristian, can you help?
> 
> 
> 

Megha,

David asked for a better description of the fix and CC stable about a month ago, did you miss his email? These should be quick to handle, are you able to send V2?

Regards,
Cristian

RE: [PATCH] sched: fix for tc_ov_enable flag position in subport structure.

[Ajmera, Megha]

> >
> > 10/01/2023 12:27, David Marchand:
> > > On Mon, Jan 9, 2023 at 3:59 PM Megha Ajmera <megha.ajmera@intel.com>
> > wrote:
> > > >
> > > > Current position of "tv_ov_enable" variable in
> > >
> > > tc_ov_enabled*
> > >
> > >
> > > > rte_sched_subport structure makes the "memory" variable unused.
> > >
> > > I did not enter the beast... but my understanding is that some
> > > object internal to rte_sched_subport currently shares location with
> > > this tc_ov_enabled field.
> > > So please find a better title and describe the impact.
> > >
> > >
> > >
> > > This is stable@dpdk.org material, isn't it?
> > >
> > >

Yes. Applicable to stable branch as well.

> 
> Megha,
> 
> David asked for a better description of the fix and CC stable about a month ago,
> did you miss his email? These should be quick to handle, are you able to send
> V2?

Hi Cristian,

I missed earlier email on this thread. Sent v2 version of patch with updated title and description.
Please have a look and review.

[PATCH v2] sched: fix for incorrect alignment of bitmap, pipe and queue structs in subport

[Megha Ajmera]

Big structures like bitmap, pipes and queues in subport are addressed
using offset of 'memory' field in subport structures. This means no other
variable should be added after 'memory' variable or else addressing of such
structs like bitmap etc. become incorrect.

Realigned tc_ov_enabled variable in subport structure.

Fixes: f5e60154ade ("sched: enable traffic class oversubscription conditionally")

Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
---
 lib/sched/rte_sched.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/sched/rte_sched.c b/lib/sched/rte_sched.c
index c91697131d..eaecd7ceb4 100644
--- a/lib/sched/rte_sched.c
+++ b/lib/sched/rte_sched.c
@@ -202,6 +202,9 @@ struct rte_sched_subport {
 	uint32_t qsize_add[RTE_SCHED_QUEUES_PER_PIPE];
 	uint32_t qsize_sum;
 
+	/* TC oversubscription activation */
+	int tc_ov_enabled;
+
 	struct rte_sched_pipe *pipe;
 	struct rte_sched_queue *queue;
 	struct rte_sched_queue_extra *queue_extra;
@@ -210,8 +213,6 @@ struct rte_sched_subport {
 	struct rte_mbuf **queue_array;
 	uint8_t memory[0] __rte_cache_aligned;
 
-	/* TC oversubscription activation */
-	int tc_ov_enabled;
 } __rte_cache_aligned;
 
 struct rte_sched_port {
-- 
2.25.1

Re: [PATCH v2] sched: fix for incorrect alignment of bitmap, pipe and queue structs in subport

[Thomas Monjalon]

07/02/2023 07:10, Megha Ajmera:
> Big structures like bitmap, pipes and queues in subport are addressed
> using offset of 'memory' field in subport structures. This means no other
> variable should be added after 'memory' variable or else addressing of such
> structs like bitmap etc. become incorrect.
> 
> Realigned tc_ov_enabled variable in subport structure.
> 
> Fixes: f5e60154ade ("sched: enable traffic class oversubscription conditionally")
> 
> Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
> Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

I bet the impact is wrongly explained, but I don't want to look deeper.
Applied.

Re: [PATCH v2] sched: fix for incorrect alignment of bitmap, pipe and queue structs in subport

[Thomas Monjalon]

19/02/2023 23:40, Thomas Monjalon:
> 07/02/2023 07:10, Megha Ajmera:
> > Big structures like bitmap, pipes and queues in subport are addressed
> > using offset of 'memory' field in subport structures. This means no other
> > variable should be added after 'memory' variable or else addressing of such
> > structs like bitmap etc. become incorrect.
> > 
> > Realigned tc_ov_enabled variable in subport structure.
> > 
> > Fixes: f5e60154ade ("sched: enable traffic class oversubscription conditionally")
> > 
> > Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
> > Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> 
> I bet the impact is wrongly explained, but I don't want to look deeper.
> Applied.

And I've added Cc: stable@dpdk.org that David and Cristian asked for.