[PATCH] rcu: remove VLAs

[PATCH] rcu: remove VLAs

[Andre Muezerie]

There are two lines that were using VLAs, which are not supported by
MSVC.

1)
../lib/rcu/rte_rcu_qsbr.c:326:12: warning: variable length array used [-Wvla]
  326 |         char data[dq->esize];
      |                   ^~~~~~~~~
2)
../lib/rcu/rte_rcu_qsbr.c:389:12: warning: variable length array used [-Wvla]
  389 |         char data[dq->esize];
      |                   ^~~~~~~~~

The short-term fix is to use alloca, to allow progress with the msvc
compatibility work.
The long-term plan involves API changes and therefore can only be applied
with a new release. This long-term plan consists of introducing some
reasonable limitation on RCU DQ element size.

Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
---
 lib/rcu/meson.build    | 7 -------
 lib/rcu/rte_rcu_qsbr.c | 6 +++---
 2 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/lib/rcu/meson.build b/lib/rcu/meson.build
index fb1f49ba63..71143f5210 100644
--- a/lib/rcu/meson.build
+++ b/lib/rcu/meson.build
@@ -11,10 +11,3 @@ sources = files('rte_rcu_qsbr.c')
 headers = files('rte_rcu_qsbr.h')
 
 deps += ['ring']
-
-# FIXME: this library was enabled for mingw target (a Windows target).
-# Relying on no_wvla_cflag would trigger a build error until the VLA in rte_rcu_qsbr.c is removed.
-# Disable the warning here for now.
-if cc.has_argument('-Wvla')
-    cflags += '-Wno-vla'
-endif
diff --git a/lib/rcu/rte_rcu_qsbr.c b/lib/rcu/rte_rcu_qsbr.c
index dbf31501a6..3f619e1607 100644
--- a/lib/rcu/rte_rcu_qsbr.c
+++ b/lib/rcu/rte_rcu_qsbr.c
@@ -323,7 +323,7 @@ int rte_rcu_qsbr_dq_enqueue(struct rte_rcu_qsbr_dq *dq, void *e)
 		return 1;
 	}
 
-	char data[dq->esize];
+	char *data = alloca(dq->esize);
 	dq_elem = (__rte_rcu_qsbr_dq_elem_t *)data;
 	/* Start the grace period */
 	dq_elem->token = rte_rcu_qsbr_start(dq->v);
@@ -386,10 +386,10 @@ rte_rcu_qsbr_dq_reclaim(struct rte_rcu_qsbr_dq *dq, unsigned int n,
 
 	cnt = 0;
 
-	char data[dq->esize];
+	char *data = alloca(dq->esize);
 	/* Check reader threads quiescent state and reclaim resources */
 	while (cnt < n &&
-		rte_ring_dequeue_bulk_elem_start(dq->r, &data,
+		rte_ring_dequeue_bulk_elem_start(dq->r, data,
 					dq->esize, 1, available) != 0) {
 		dq_elem = (__rte_rcu_qsbr_dq_elem_t *)data;
 
-- 
2.48.1.vfs.0.0

Re: [PATCH] rcu: remove VLAs

[David Marchand]

On Fri, Mar 7, 2025 at 2:40 AM Andre Muezerie
<andremue@linux.microsoft.com> wrote:
>
> There are two lines that were using VLAs, which are not supported by
> MSVC.
>
> 1)
> ../lib/rcu/rte_rcu_qsbr.c:326:12: warning: variable length array used [-Wvla]
>   326 |         char data[dq->esize];
>       |                   ^~~~~~~~~
> 2)
> ../lib/rcu/rte_rcu_qsbr.c:389:12: warning: variable length array used [-Wvla]
>   389 |         char data[dq->esize];
>       |                   ^~~~~~~~~
>
> The short-term fix is to use alloca, to allow progress with the msvc
> compatibility work.
> The long-term plan involves API changes and therefore can only be applied
> with a new release. This long-term plan consists of introducing some
> reasonable limitation on RCU DQ element size.
>
> Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>

Honnappa, any objection?



-- 
David Marchand

Re: [PATCH] rcu: remove VLAs

[David Marchand]

Hello,

On Thu, Apr 10, 2025 at 1:39 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Fri, Mar 7, 2025 at 2:40 AM Andre Muezerie
> <andremue@linux.microsoft.com> wrote:
> >
> > There are two lines that were using VLAs, which are not supported by
> > MSVC.
> >
> > 1)
> > ../lib/rcu/rte_rcu_qsbr.c:326:12: warning: variable length array used [-Wvla]
> >   326 |         char data[dq->esize];
> >       |                   ^~~~~~~~~
> > 2)
> > ../lib/rcu/rte_rcu_qsbr.c:389:12: warning: variable length array used [-Wvla]
> >   389 |         char data[dq->esize];
> >       |                   ^~~~~~~~~
> >
> > The short-term fix is to use alloca, to allow progress with the msvc
> > compatibility work.
> > The long-term plan involves API changes and therefore can only be applied
> > with a new release. This long-term plan consists of introducing some
> > reasonable limitation on RCU DQ element size.
> >
> > Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
>
> Honnappa, any objection?

I'll take silence as a no.

Andre, you mention long-term plan, if you plan to do this change in
25.11, now would be a good time to prepare a deprecation notice on
this topic.


-- 
David Marchand

Re: [PATCH] rcu: remove VLAs

[David Marchand]

Andre,

On Fri, Mar 7, 2025 at 2:40 AM Andre Muezerie
<andremue@linux.microsoft.com> wrote:
>
> There are two lines that were using VLAs, which are not supported by
> MSVC.
>
> 1)
> ../lib/rcu/rte_rcu_qsbr.c:326:12: warning: variable length array used [-Wvla]
>   326 |         char data[dq->esize];
>       |                   ^~~~~~~~~
> 2)
> ../lib/rcu/rte_rcu_qsbr.c:389:12: warning: variable length array used [-Wvla]
>   389 |         char data[dq->esize];
>       |                   ^~~~~~~~~
>
> The short-term fix is to use alloca, to allow progress with the msvc
> compatibility work.
> The long-term plan involves API changes and therefore can only be applied
> with a new release. This long-term plan consists of introducing some
> reasonable limitation on RCU DQ element size.
>
> Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>

Afaiu, enabling RCU with MSVC depends on this current patch and a fix
in test/ipfrag:
https://patchwork.dpdk.org/project/dpdk/patch/1741313581-14300-1-git-send-email-andremue@linux.microsoft.com/.
Is there anything else missing?

If not, then I would be for doing this enabling as part of this
current patch (I can do this when applying).
WDYT?


-- 
David Marchand

Re: [PATCH] rcu: remove VLAs

[Andre Muezerie]

On Fri, May 16, 2025 at 11:22:05AM +0200, David Marchand wrote:
> Andre,
> 
> On Fri, Mar 7, 2025 at 2:40 AM Andre Muezerie
> <andremue@linux.microsoft.com> wrote:
> >
> > There are two lines that were using VLAs, which are not supported by
> > MSVC.
> >
> > 1)
> > ../lib/rcu/rte_rcu_qsbr.c:326:12: warning: variable length array used [-Wvla]
> >   326 |         char data[dq->esize];
> >       |                   ^~~~~~~~~
> > 2)
> > ../lib/rcu/rte_rcu_qsbr.c:389:12: warning: variable length array used [-Wvla]
> >   389 |         char data[dq->esize];
> >       |                   ^~~~~~~~~
> >
> > The short-term fix is to use alloca, to allow progress with the msvc
> > compatibility work.
> > The long-term plan involves API changes and therefore can only be applied
> > with a new release. This long-term plan consists of introducing some
> > reasonable limitation on RCU DQ element size.
> >
> > Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
> 
> Afaiu, enabling RCU with MSVC depends on this current patch and a fix
> in test/ipfrag:
> https://patchwork.dpdk.org/project/dpdk/patch/1741313581-14300-1-git-send-email-andremue@linux.microsoft.com/.
> Is there anything else missing?
> 
> If not, then I would be for doing this enabling as part of this
> current patch (I can do this when applying).
> WDYT?
> 

You're correct. When enabling RCU lib, test_ipfrag is build and fails if the mentioned
patch is not applied yet. If you apply that patch first, it would be great if you
could enable RCU on Windows as part of this patch. Nothing else should be missing.

I had not enabled it before to not break the CI build.

> 
> -- 
> David Marchand

Re: [PATCH] rcu: remove VLAs

[David Marchand]

On Fri, Mar 7, 2025 at 2:40 AM Andre Muezerie
<andremue@linux.microsoft.com> wrote:
>
> There are two lines that were using VLAs, which are not supported by
> MSVC.
>
> 1)
> ../lib/rcu/rte_rcu_qsbr.c:326:12: warning: variable length array used [-Wvla]
>   326 |         char data[dq->esize];
>       |                   ^~~~~~~~~
> 2)
> ../lib/rcu/rte_rcu_qsbr.c:389:12: warning: variable length array used [-Wvla]
>   389 |         char data[dq->esize];
>       |                   ^~~~~~~~~
>
> The short-term fix is to use alloca, to allow progress with the msvc
> compatibility work.
> The long-term plan involves API changes and therefore can only be applied
> with a new release. This long-term plan consists of introducing some
> reasonable limitation on RCU DQ element size.
>
> Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>

Applied with enabling RCU compilation on MSVC.
Thanks.


-- 
David Marchand

Re: [PATCH] rcu: remove VLAs

[David Marchand]

On Fri, May 16, 2025 at 3:09 PM Andre Muezerie
<andremue@linux.microsoft.com> wrote:
>
> On Fri, May 16, 2025 at 11:22:05AM +0200, David Marchand wrote:
> > Andre,
> >
> > On Fri, Mar 7, 2025 at 2:40 AM Andre Muezerie
> > <andremue@linux.microsoft.com> wrote:
> > >
> > > There are two lines that were using VLAs, which are not supported by
> > > MSVC.
> > >
> > > 1)
> > > ../lib/rcu/rte_rcu_qsbr.c:326:12: warning: variable length array used [-Wvla]
> > >   326 |         char data[dq->esize];
> > >       |                   ^~~~~~~~~
> > > 2)
> > > ../lib/rcu/rte_rcu_qsbr.c:389:12: warning: variable length array used [-Wvla]
> > >   389 |         char data[dq->esize];
> > >       |                   ^~~~~~~~~
> > >
> > > The short-term fix is to use alloca, to allow progress with the msvc
> > > compatibility work.
> > > The long-term plan involves API changes and therefore can only be applied
> > > with a new release. This long-term plan consists of introducing some
> > > reasonable limitation on RCU DQ element size.
> > >
> > > Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
> >
> > Afaiu, enabling RCU with MSVC depends on this current patch and a fix
> > in test/ipfrag:
> > https://patchwork.dpdk.org/project/dpdk/patch/1741313581-14300-1-git-send-email-andremue@linux.microsoft.com/.
> > Is there anything else missing?
> >
> > If not, then I would be for doing this enabling as part of this
> > current patch (I can do this when applying).
> > WDYT?
> >
>
> You're correct. When enabling RCU lib, test_ipfrag is build and fails if the mentioned
> patch is not applied yet. If you apply that patch first, it would be great if you
> could enable RCU on Windows as part of this patch. Nothing else should be missing.
>
> I had not enabled it before to not break the CI build.

Ok, I did my checks and it looks fine, so I squashed the mention
change as part of this patch.

I see patches on lpm and fib library for which I have similar questions.
I must stop merging for today but could you double check those
libraries can be enabled too?

This way it will be easier for me next week.


Thanks.

-- 
David Marchand

Re: [PATCH] rcu: remove VLAs

[Andre Muezerie]

On Fri, May 16, 2025 at 03:30:23PM +0200, David Marchand wrote:
> On Fri, May 16, 2025 at 3:09 PM Andre Muezerie
> <andremue@linux.microsoft.com> wrote:
> >
> > On Fri, May 16, 2025 at 11:22:05AM +0200, David Marchand wrote:
> > > Andre,
> > >
> > > On Fri, Mar 7, 2025 at 2:40 AM Andre Muezerie
> > > <andremue@linux.microsoft.com> wrote:
> > > >
> > > > There are two lines that were using VLAs, which are not supported by
> > > > MSVC.
> > > >
> > > > 1)
> > > > ../lib/rcu/rte_rcu_qsbr.c:326:12: warning: variable length array used [-Wvla]
> > > >   326 |         char data[dq->esize];
> > > >       |                   ^~~~~~~~~
> > > > 2)
> > > > ../lib/rcu/rte_rcu_qsbr.c:389:12: warning: variable length array used [-Wvla]
> > > >   389 |         char data[dq->esize];
> > > >       |                   ^~~~~~~~~
> > > >
> > > > The short-term fix is to use alloca, to allow progress with the msvc
> > > > compatibility work.
> > > > The long-term plan involves API changes and therefore can only be applied
> > > > with a new release. This long-term plan consists of introducing some
> > > > reasonable limitation on RCU DQ element size.
> > > >
> > > > Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
> > >
> > > Afaiu, enabling RCU with MSVC depends on this current patch and a fix
> > > in test/ipfrag:
> > > https://patchwork.dpdk.org/project/dpdk/patch/1741313581-14300-1-git-send-email-andremue@linux.microsoft.com/.
> > > Is there anything else missing?
> > >
> > > If not, then I would be for doing this enabling as part of this
> > > current patch (I can do this when applying).
> > > WDYT?
> > >
> >
> > You're correct. When enabling RCU lib, test_ipfrag is build and fails if the mentioned
> > patch is not applied yet. If you apply that patch first, it would be great if you
> > could enable RCU on Windows as part of this patch. Nothing else should be missing.
> >
> > I had not enabled it before to not break the CI build.
> 
> Ok, I did my checks and it looks fine, so I squashed the mention
> change as part of this patch.
> 
> I see patches on lpm and fib library for which I have similar questions.
> I must stop merging for today but could you double check those
> libraries can be enabled too?
> 
> This way it will be easier for me next week.
> 

I updated both patches to also enable the build when using MSVC since there's
nothing blocking that anymore.

> 
> Thanks.
> 
> -- 
> David Marchand