C++20 report error at file rte_spinlock.h

C++20 report error at file rte_spinlock.h

[Zhou, Xiangyun]

[-- Attachment #1.1: Type: text/plain, Size: 1327 bytes --]

Dear dpdk dev,

I'm using dpdk 21.11 LTS, when compile my program with CPP flag "-std=c++20", the compiler report below errors. After checking file rte_spinlock.h, I think the error report by compiler is valid, there should be a potential issue when using functions rte_spinlock_recursive_lock, rte_spinlock_recursive_unlock and rte_spinlock_recursive_trylock in multi-thread, we could either remove "volatile" definition to ask users to handle the multi-thread issue, or using atomic operatings instead of self-increment and self-decrement.


/home/dpdk/lib/eal/include/generic/rte_spinlock.h:221:12: error: increment of object of volatile-qualified type 'volatile int' is deprecated [-Werror,-Wdeprecated-volatile]
        slr->count++;
                  ^
/home/dpdk/lib/eal/include/generic/rte_spinlock.h:231:6: error: decrement of object of volatile-qualified type 'volatile int' is deprecated [-Werror,-Wdeprecated-volatile]
        if (--(slr->count) == 0) {
            ^
/home/dpdk/lib/eal/include/generic/rte_spinlock.h:255:12: error: increment of object of volatile-qualified type 'volatile int' is deprecated [-Werror,-Wdeprecated-volatile]
        slr->count++;



BR,

[A close up of a sign  Description automatically generated]

Xiangyun Zhou
NPG SWE SWA
Intel Corporation  |  intel.com


[-- Attachment #1.2: Type: text/html, Size: 5712 bytes --]

[-- Attachment #2: image001.png --]
[-- Type: image/png, Size: 6602 bytes --]

Re: C++20 report error at file rte_spinlock.h

[Tyler Retzlaff]

On Tue, Dec 13, 2022 at 06:11:06AM +0000, Zhou, Xiangyun wrote:
> Dear dpdk dev,
> 
> I'm using dpdk 21.11 LTS, when compile my program with CPP flag "-std=c++20", the compiler report below errors. After checking file rte_spinlock.h, I think the error report by compiler is valid, there should be a potential issue when using functions rte_spinlock_recursive_lock, rte_spinlock_recursive_unlock and rte_spinlock_recursive_trylock in multi-thread, we could either remove "volatile" definition to ask users to handle the multi-thread issue, or using atomic operatings instead of self-increment and self-decrement.
> 
> 
> /home/dpdk/lib/eal/include/generic/rte_spinlock.h:221:12: error: increment of object of volatile-qualified type 'volatile int' is deprecated [-Werror,-Wdeprecated-volatile]
>         slr->count++;
>                   ^
> /home/dpdk/lib/eal/include/generic/rte_spinlock.h:231:6: error: decrement of object of volatile-qualified type 'volatile int' is deprecated [-Werror,-Wdeprecated-volatile]
>         if (--(slr->count) == 0) {
>             ^
> /home/dpdk/lib/eal/include/generic/rte_spinlock.h:255:12: error: increment of object of volatile-qualified type 'volatile int' is deprecated [-Werror,-Wdeprecated-volatile]
>         slr->count++;
> 

i have work in progress to optionally use standard atomics but in the
meantime the correct thing to do here is to use the gcc builtins that
match the requirements of the c++11 memory model.

the code should be converted to use __atomic_fetch_{add,sub} or
__atomic_{add,sub}_fetch as appropriate.

ty.

RE: C++20 report error at file rte_spinlock.h

[Konstantin Ananyev]

> On Tue, Dec 13, 2022 at 06:11:06AM +0000, Zhou, Xiangyun wrote:
> > Dear dpdk dev,
> >
> > I'm using dpdk 21.11 LTS, when compile my program with CPP flag "-std=c++20", the compiler report below errors. After checking file
> rte_spinlock.h, I think the error report by compiler is valid, there should be a potential issue when using functions
> rte_spinlock_recursive_lock, rte_spinlock_recursive_unlock and rte_spinlock_recursive_trylock in multi-thread, we could either
> remove "volatile" definition to ask users to handle the multi-thread issue, or using atomic operatings instead of self-increment and
> self-decrement.
> >
> >
> > /home/dpdk/lib/eal/include/generic/rte_spinlock.h:221:12: error: increment of object of volatile-qualified type 'volatile int' is
> deprecated [-Werror,-Wdeprecated-volatile]
> >         slr->count++;
> >                   ^
> > /home/dpdk/lib/eal/include/generic/rte_spinlock.h:231:6: error: decrement of object of volatile-qualified type 'volatile int' is
> deprecated [-Werror,-Wdeprecated-volatile]
> >         if (--(slr->count) == 0) {
> >             ^
> > /home/dpdk/lib/eal/include/generic/rte_spinlock.h:255:12: error: increment of object of volatile-qualified type 'volatile int' is
> deprecated [-Werror,-Wdeprecated-volatile]
> >         slr->count++;
> >
> 
> i have work in progress to optionally use standard atomics but in the
> meantime the correct thing to do here is to use the gcc builtins that
> match the requirements of the c++11 memory model.
> 
> the code should be converted to use __atomic_fetch_{add,sub} or
> __atomic_{add,sub}_fetch as appropriate.
> 
> ty.

From looking at the code, I don't think it is necessary:
both 'user' and 'count' supposed to be protected by 'sl'.
In fact, it looks safe just to remove 'volatile' qualifier here.
 

RE: C++20 report error at file rte_spinlock.h

[Zhou, Xiangyun]

Thanks very much  for Konstantin and Tyler's analyzing.

I agree that removal of 'volatile' is enough. A spinlock has already used to protect these variables.

-----Original Message-----
From: Konstantin Ananyev <konstantin.ananyev@huawei.com> 
Sent: Tuesday, December 20, 2022 12:51 AM
To: Tyler Retzlaff <roretzla@linux.microsoft.com>; Zhou, Xiangyun <xiangyun.zhou@intel.com>
Cc: dev@dpdk.org; Xu, Bowen <bowen.xu@intel.com>
Subject: RE: C++20 report error at file rte_spinlock.h


> On Tue, Dec 13, 2022 at 06:11:06AM +0000, Zhou, Xiangyun wrote:
> > Dear dpdk dev,
> >
> > I'm using dpdk 21.11 LTS, when compile my program with CPP flag 
> > "-std=c++20", the compiler report below errors. After checking file
> rte_spinlock.h, I think the error report by compiler is valid, there 
> should be a potential issue when using functions 
> rte_spinlock_recursive_lock, rte_spinlock_recursive_unlock and 
> rte_spinlock_recursive_trylock in multi-thread, we could either remove "volatile" definition to ask users to handle the multi-thread issue, or using atomic operatings instead of self-increment and self-decrement.
> >
> >
> > /home/dpdk/lib/eal/include/generic/rte_spinlock.h:221:12: error: 
> > increment of object of volatile-qualified type 'volatile int' is
> deprecated [-Werror,-Wdeprecated-volatile]
> >         slr->count++;
> >                   ^
> > /home/dpdk/lib/eal/include/generic/rte_spinlock.h:231:6: error: 
> > decrement of object of volatile-qualified type 'volatile int' is
> deprecated [-Werror,-Wdeprecated-volatile]
> >         if (--(slr->count) == 0) {
> >             ^
> > /home/dpdk/lib/eal/include/generic/rte_spinlock.h:255:12: error: 
> > increment of object of volatile-qualified type 'volatile int' is
> deprecated [-Werror,-Wdeprecated-volatile]
> >         slr->count++;
> >
> 
> i have work in progress to optionally use standard atomics but in the 
> meantime the correct thing to do here is to use the gcc builtins that 
> match the requirements of the c++11 memory model.
> 
> the code should be converted to use __atomic_fetch_{add,sub} or 
> __atomic_{add,sub}_fetch as appropriate.
> 
> ty.

From looking at the code, I don't think it is necessary:
both 'user' and 'count' supposed to be protected by 'sl'.
In fact, it looks safe just to remove 'volatile' qualifier here.
 

Re: C++20 report error at file rte_spinlock.h

[Stephen Hemminger]

On Tue, 20 Dec 2022 02:11:42 +0000
"Zhou, Xiangyun" <xiangyun.zhou@intel.com> wrote:

> Thanks very much  for Konstantin and Tyler's analyzing.
> 
> I agree that removal of 'volatile' is enough. A spinlock has already used to protect these variables.
> 
> -----Original Message-----
> From: Konstantin Ananyev <konstantin.ananyev@huawei.com> 
> Sent: Tuesday, December 20, 2022 12:51 AM
> To: Tyler Retzlaff <roretzla@linux.microsoft.com>; Zhou, Xiangyun <xiangyun.zhou@intel.com>
> Cc: dev@dpdk.org; Xu, Bowen <bowen.xu@intel.com>
> Subject: RE: C++20 report error at file rte_spinlock.h
> 
> 
> > On Tue, Dec 13, 2022 at 06:11:06AM +0000, Zhou, Xiangyun wrote:  
> > > Dear dpdk dev,
> > >
> > > I'm using dpdk 21.11 LTS, when compile my program with CPP flag 
> > > "-std=c++20", the compiler report below errors. After checking file  
> > rte_spinlock.h, I think the error report by compiler is valid, there 
> > should be a potential issue when using functions 
> > rte_spinlock_recursive_lock, rte_spinlock_recursive_unlock and 
> > rte_spinlock_recursive_trylock in multi-thread, we could either remove "volatile" definition to ask users to handle the multi-thread issue, or using atomic operatings instead of self-increment and self-decrement.  
> > >
> > >
> > > /home/dpdk/lib/eal/include/generic/rte_spinlock.h:221:12: error: 
> > > increment of object of volatile-qualified type 'volatile int' is  
> > deprecated [-Werror,-Wdeprecated-volatile]  
> > >         slr->count++;
> > >                   ^
> > > /home/dpdk/lib/eal/include/generic/rte_spinlock.h:231:6: error: 
> > > decrement of object of volatile-qualified type 'volatile int' is  
> > deprecated [-Werror,-Wdeprecated-volatile]  
> > >         if (--(slr->count) == 0) {
> > >             ^
> > > /home/dpdk/lib/eal/include/generic/rte_spinlock.h:255:12: error: 
> > > increment of object of volatile-qualified type 'volatile int' is  
> > deprecated [-Werror,-Wdeprecated-volatile]  
> > >         slr->count++;
> > >  
> > 
> > i have work in progress to optionally use standard atomics but in the 
> > meantime the correct thing to do here is to use the gcc builtins that 
> > match the requirements of the c++11 memory model.
> > 
> > the code should be converted to use __atomic_fetch_{add,sub} or 
> > __atomic_{add,sub}_fetch as appropriate.
> > 
> > ty.  
> 
> From looking at the code, I don't think it is necessary:
> both 'user' and 'count' supposed to be protected by 'sl'.
> In fact, it looks safe just to remove 'volatile' qualifier here.
>  
> 

I noticed a couple of other documentation errors here.

The intro comment says this a read-write lock (and it isn't).
Probably copy/paste error.

The user field says is is "core id" but it really is thread id.