| Message ID | 20201021114912.2552-1-elibr@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,dpdk-latest,1/1] sparse: Fix __ATOMIC_* redefinition errors | expand |
> In sparse commit [1], __ATOMIC_* defines were introduced, which cause > redefinition errors. Wrap OVS defines with #ifndef to fix it. Thanks for this Eli as well as the testing, was a bit confused myself as the community CI was passing yesterday initially with RC1, so good catch. One minor comment below. > > [1] > https://git.kernel.org/pub/scm/devel/sparse/sparse.git/commit/?id=cf8f104749 > f5bca36852989297af8cc19ff24d5f > > Tested-at: https://travis-ci.org/github/elibritstein/OVS/builds/737660375 > Signed-off-by: Eli Britstein <elibr@nvidia.com> > --- > include/sparse/rte_mbuf.h | 8 ++++++++ > include/sparse/rte_trace_point.h | 4 ++++ > 2 files changed, 12 insertions(+) > > diff --git a/include/sparse/rte_mbuf.h b/include/sparse/rte_mbuf.h > index ee461f91e..172c9954a 100644 > --- a/include/sparse/rte_mbuf.h > +++ b/include/sparse/rte_mbuf.h > @@ -18,10 +18,18 @@ > #endif > > /* sparse doesn't know about gcc atomic builtins. */ > +#ifndef __ATOMIC_ACQ_REL > #define __ATOMIC_ACQ_REL 0 > +#endif > +#ifndef __ATOMIC_RELAXED > #define __ATOMIC_RELAXED 1 > +#endif > +#ifndef __atomic_add_fetch > #define __atomic_add_fetch(p, val, memorder) (*(p) = *(p) + (val)) So in the sparse patch https://git.kernel.org/pub/scm/devel/sparse/sparse.git/commit/?id=cf8f104749f5bca36852989297af8cc19ff24d5f I see the changes to the ACQ_REL and ATOMIC_RELAXED, but no mention of the atomic_add_fetch, for my own curiosity must the fetch atomic fetch and gather operations also be wrapped because of the sparse patch or is this more an effort future proof and keep the definitions uniform in OVS? Same query WRT the atomic load operation below. Thanks Ian > +#endif > +#ifndef __atomic_store_n > #define __atomic_store_n(p, val, memorder) (*(p) = (val)) > +#endif > > /* Get actual <rte_mbuf.h> definitions for us to annotate and build on. */ > #include_next <rte_mbuf.h> > diff --git a/include/sparse/rte_trace_point.h b/include/sparse/rte_trace_point.h > index c28f1c941..94bd54b25 100644 > --- a/include/sparse/rte_trace_point.h > +++ b/include/sparse/rte_trace_point.h > @@ -18,8 +18,12 @@ > #endif > > /* sparse doesn't know about gcc atomic builtins. */ > +#ifndef __ATOMIC_ACQUIRE > #define __ATOMIC_ACQUIRE 0 > +#endif > +#ifndef __atomic_load_n > #define __atomic_load_n(p, memorder) *(p) > +#endif > > /* Get actual <rte_trace_point.h> definitions for us to annotate and > * build on. */ > -- > 2.28.0.546.g385c171
On 10/21/2020 3:17 PM, Stokes, Ian wrote: > External email: Use caution opening links or attachments > > >> In sparse commit [1], __ATOMIC_* defines were introduced, which cause >> redefinition errors. Wrap OVS defines with #ifndef to fix it. > Thanks for this Eli as well as the testing, was a bit confused myself as the community CI was passing yesterday initially with RC1, so good catch. > > One minor comment below. >> [1] >> https://git.kernel.org/pub/scm/devel/sparse/sparse.git/commit/?id=cf8f104749 >> f5bca36852989297af8cc19ff24d5f >> >> Tested-at: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Fgithub%2Felibritstein%2FOVS%2Fbuilds%2F737660375&data=04%7C01%7Celibr%40nvidia.com%7C6a6306041ce044bcee7008d875bb4b30%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637388794570571904%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=bVccwZY8zXQvschI52O7w565dLu7KcaPvQWbExgsWL0%3D&reserved=0 >> Signed-off-by: Eli Britstein <elibr@nvidia.com> >> --- >> include/sparse/rte_mbuf.h | 8 ++++++++ >> include/sparse/rte_trace_point.h | 4 ++++ >> 2 files changed, 12 insertions(+) >> >> diff --git a/include/sparse/rte_mbuf.h b/include/sparse/rte_mbuf.h >> index ee461f91e..172c9954a 100644 >> --- a/include/sparse/rte_mbuf.h >> +++ b/include/sparse/rte_mbuf.h >> @@ -18,10 +18,18 @@ >> #endif >> >> /* sparse doesn't know about gcc atomic builtins. */ >> +#ifndef __ATOMIC_ACQ_REL >> #define __ATOMIC_ACQ_REL 0 >> +#endif >> +#ifndef __ATOMIC_RELAXED >> #define __ATOMIC_RELAXED 1 >> +#endif >> +#ifndef __atomic_add_fetch >> #define __atomic_add_fetch(p, val, memorder) (*(p) = *(p) + (val)) > So in the sparse patch https://git.kernel.org/pub/scm/devel/sparse/sparse.git/commit/?id=cf8f104749f5bca36852989297af8cc19ff24d5f > > I see the changes to the ACQ_REL and ATOMIC_RELAXED, but no mention of the atomic_add_fetch, for my own curiosity must the fetch atomic fetch and gather operations also be wrapped because of the sparse patch or is this more an effort future proof and keep the definitions uniform in OVS? Same query WRT the atomic load operation below. I took the liberty to wrap the others as well, for future proof. > > Thanks > Ian > >> +#endif >> +#ifndef __atomic_store_n >> #define __atomic_store_n(p, val, memorder) (*(p) = (val)) >> +#endif >> >> /* Get actual <rte_mbuf.h> definitions for us to annotate and build on. */ >> #include_next <rte_mbuf.h> >> diff --git a/include/sparse/rte_trace_point.h b/include/sparse/rte_trace_point.h >> index c28f1c941..94bd54b25 100644 >> --- a/include/sparse/rte_trace_point.h >> +++ b/include/sparse/rte_trace_point.h >> @@ -18,8 +18,12 @@ >> #endif >> >> /* sparse doesn't know about gcc atomic builtins. */ >> +#ifndef __ATOMIC_ACQUIRE >> #define __ATOMIC_ACQUIRE 0 >> +#endif >> +#ifndef __atomic_load_n >> #define __atomic_load_n(p, memorder) *(p) >> +#endif >> >> /* Get actual <rte_trace_point.h> definitions for us to annotate and >> * build on. */ >> -- >> 2.28.0.546.g385c171
On 10/21/20 2:17 PM, Stokes, Ian wrote: >> In sparse commit [1], __ATOMIC_* defines were introduced, which cause >> redefinition errors. Wrap OVS defines with #ifndef to fix it. > > Thanks for this Eli as well as the testing, was a bit confused myself as the community CI was passing yesterday initially with RC1, so good catch. > > One minor comment below. >> >> [1] >> https://git.kernel.org/pub/scm/devel/sparse/sparse.git/commit/?id=cf8f104749 >> f5bca36852989297af8cc19ff24d5f >> >> Tested-at: https://travis-ci.org/github/elibritstein/OVS/builds/737660375 >> Signed-off-by: Eli Britstein <elibr@nvidia.com> >> --- >> include/sparse/rte_mbuf.h | 8 ++++++++ >> include/sparse/rte_trace_point.h | 4 ++++ >> 2 files changed, 12 insertions(+) >> >> diff --git a/include/sparse/rte_mbuf.h b/include/sparse/rte_mbuf.h >> index ee461f91e..172c9954a 100644 >> --- a/include/sparse/rte_mbuf.h >> +++ b/include/sparse/rte_mbuf.h >> @@ -18,10 +18,18 @@ >> #endif >> >> /* sparse doesn't know about gcc atomic builtins. */ >> +#ifndef __ATOMIC_ACQ_REL >> #define __ATOMIC_ACQ_REL 0 >> +#endif >> +#ifndef __ATOMIC_RELAXED >> #define __ATOMIC_RELAXED 1 >> +#endif >> +#ifndef __atomic_add_fetch >> #define __atomic_add_fetch(p, val, memorder) (*(p) = *(p) + (val)) > > So in the sparse patch https://git.kernel.org/pub/scm/devel/sparse/sparse.git/commit/?id=cf8f104749f5bca36852989297af8cc19ff24d5f > > I see the changes to the ACQ_REL and ATOMIC_RELAXED, but no mention of the atomic_add_fetch, for my own curiosity must the fetch atomic fetch and gather operations also be wrapped because of the sparse patch or is this more an effort future proof and keep the definitions uniform in OVS? Same query WRT the atomic load operation below. There is another recent patch: https://git.kernel.org/pub/scm/devel/sparse/sparse.git/commit/?id=7cdf84691f33e4fc6a0198e1a04137993f3a37ad This one introduces support for __atomic_add_fetch and friends. I didn't test it, though. > > Thanks > Ian > >> +#endif >> +#ifndef __atomic_store_n >> #define __atomic_store_n(p, val, memorder) (*(p) = (val)) >> +#endif >> >> /* Get actual <rte_mbuf.h> definitions for us to annotate and build on. */ >> #include_next <rte_mbuf.h> >> diff --git a/include/sparse/rte_trace_point.h b/include/sparse/rte_trace_point.h >> index c28f1c941..94bd54b25 100644 >> --- a/include/sparse/rte_trace_point.h >> +++ b/include/sparse/rte_trace_point.h >> @@ -18,8 +18,12 @@ >> #endif >> >> /* sparse doesn't know about gcc atomic builtins. */ >> +#ifndef __ATOMIC_ACQUIRE >> #define __ATOMIC_ACQUIRE 0 >> +#endif >> +#ifndef __atomic_load_n >> #define __atomic_load_n(p, memorder) *(p) >> +#endif >> >> /* Get actual <rte_trace_point.h> definitions for us to annotate and >> * build on. */ >> -- >> 2.28.0.546.g385c171 >
On 10/21/20 2:26 PM, Ilya Maximets wrote: > On 10/21/20 2:17 PM, Stokes, Ian wrote: >>> In sparse commit [1], __ATOMIC_* defines were introduced, which cause >>> redefinition errors. Wrap OVS defines with #ifndef to fix it. >> >> Thanks for this Eli as well as the testing, was a bit confused myself as the community CI was passing yesterday initially with RC1, so good catch. >> >> One minor comment below. >>> >>> [1] >>> https://git.kernel.org/pub/scm/devel/sparse/sparse.git/commit/?id=cf8f104749 >>> f5bca36852989297af8cc19ff24d5f >>> >>> Tested-at: https://travis-ci.org/github/elibritstein/OVS/builds/737660375 >>> Signed-off-by: Eli Britstein <elibr@nvidia.com> >>> --- >>> include/sparse/rte_mbuf.h | 8 ++++++++ >>> include/sparse/rte_trace_point.h | 4 ++++ >>> 2 files changed, 12 insertions(+) >>> >>> diff --git a/include/sparse/rte_mbuf.h b/include/sparse/rte_mbuf.h >>> index ee461f91e..172c9954a 100644 >>> --- a/include/sparse/rte_mbuf.h >>> +++ b/include/sparse/rte_mbuf.h >>> @@ -18,10 +18,18 @@ >>> #endif >>> >>> /* sparse doesn't know about gcc atomic builtins. */ >>> +#ifndef __ATOMIC_ACQ_REL >>> #define __ATOMIC_ACQ_REL 0 >>> +#endif >>> +#ifndef __ATOMIC_RELAXED >>> #define __ATOMIC_RELAXED 1 >>> +#endif >>> +#ifndef __atomic_add_fetch >>> #define __atomic_add_fetch(p, val, memorder) (*(p) = *(p) + (val)) >> >> So in the sparse patch https://git.kernel.org/pub/scm/devel/sparse/sparse.git/commit/?id=cf8f104749f5bca36852989297af8cc19ff24d5f >> >> I see the changes to the ACQ_REL and ATOMIC_RELAXED, but no mention of the atomic_add_fetch, for my own curiosity must the fetch atomic fetch and gather operations also be wrapped because of the sparse patch or is this more an effort future proof and keep the definitions uniform in OVS? Same query WRT the atomic load operation below. > > There is another recent patch: > https://git.kernel.org/pub/scm/devel/sparse/sparse.git/commit/?id=7cdf84691f33e4fc6a0198e1a04137993f3a37ad > This one introduces support for __atomic_add_fetch and friends. > I didn't test it, though. OTOH, __atomic_add_fetch is a builtin function, so, IIUC, ifndef will not work. > >> >> Thanks >> Ian >> >>> +#endif >>> +#ifndef __atomic_store_n >>> #define __atomic_store_n(p, val, memorder) (*(p) = (val)) >>> +#endif >>> >>> /* Get actual <rte_mbuf.h> definitions for us to annotate and build on. */ >>> #include_next <rte_mbuf.h> >>> diff --git a/include/sparse/rte_trace_point.h b/include/sparse/rte_trace_point.h >>> index c28f1c941..94bd54b25 100644 >>> --- a/include/sparse/rte_trace_point.h >>> +++ b/include/sparse/rte_trace_point.h >>> @@ -18,8 +18,12 @@ >>> #endif >>> >>> /* sparse doesn't know about gcc atomic builtins. */ >>> +#ifndef __ATOMIC_ACQUIRE >>> #define __ATOMIC_ACQUIRE 0 >>> +#endif >>> +#ifndef __atomic_load_n >>> #define __atomic_load_n(p, memorder) *(p) >>> +#endif >>> >>> /* Get actual <rte_trace_point.h> definitions for us to annotate and >>> * build on. */ >>> -- >>> 2.28.0.546.g385c171 >> >
On 10/21/2020 3:31 PM, Ilya Maximets wrote: > External email: Use caution opening links or attachments > > > On 10/21/20 2:26 PM, Ilya Maximets wrote: >> On 10/21/20 2:17 PM, Stokes, Ian wrote: >>>> In sparse commit [1], __ATOMIC_* defines were introduced, which cause >>>> redefinition errors. Wrap OVS defines with #ifndef to fix it. >>> Thanks for this Eli as well as the testing, was a bit confused myself as the community CI was passing yesterday initially with RC1, so good catch. >>> >>> One minor comment below. >>>> [1] >>>> https://git.kernel.org/pub/scm/devel/sparse/sparse.git/commit/?id=cf8f104749 >>>> f5bca36852989297af8cc19ff24d5f >>>> >>>> Tested-at: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Fgithub%2Felibritstein%2FOVS%2Fbuilds%2F737660375&data=04%7C01%7Celibr%40nvidia.com%7Cfdc54e0b4794474feeed08d875bd528c%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637388803280397169%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=8crkpROiXeyXdlesQuYUix0jclM3EwEpXpCdw2LgTQg%3D&reserved=0 >>>> Signed-off-by: Eli Britstein <elibr@nvidia.com> >>>> --- >>>> include/sparse/rte_mbuf.h | 8 ++++++++ >>>> include/sparse/rte_trace_point.h | 4 ++++ >>>> 2 files changed, 12 insertions(+) >>>> >>>> diff --git a/include/sparse/rte_mbuf.h b/include/sparse/rte_mbuf.h >>>> index ee461f91e..172c9954a 100644 >>>> --- a/include/sparse/rte_mbuf.h >>>> +++ b/include/sparse/rte_mbuf.h >>>> @@ -18,10 +18,18 @@ >>>> #endif >>>> >>>> /* sparse doesn't know about gcc atomic builtins. */ >>>> +#ifndef __ATOMIC_ACQ_REL >>>> #define __ATOMIC_ACQ_REL 0 >>>> +#endif >>>> +#ifndef __ATOMIC_RELAXED >>>> #define __ATOMIC_RELAXED 1 >>>> +#endif >>>> +#ifndef __atomic_add_fetch >>>> #define __atomic_add_fetch(p, val, memorder) (*(p) = *(p) + (val)) >>> So in the sparse patch https://git.kernel.org/pub/scm/devel/sparse/sparse.git/commit/?id=cf8f104749f5bca36852989297af8cc19ff24d5f >>> >>> I see the changes to the ACQ_REL and ATOMIC_RELAXED, but no mention of the atomic_add_fetch, for my own curiosity must the fetch atomic fetch and gather operations also be wrapped because of the sparse patch or is this more an effort future proof and keep the definitions uniform in OVS? Same query WRT the atomic load operation below. >> There is another recent patch: >> https://git.kernel.org/pub/scm/devel/sparse/sparse.git/commit/?id=7cdf84691f33e4fc6a0198e1a04137993f3a37ad >> This one introduces support for __atomic_add_fetch and friends. >> I didn't test it, though. > OTOH, __atomic_add_fetch is a builtin function, so, IIUC, ifndef will not work. I added ifndef as they have define in OVS. We can omit them at all as in the previous revert series, but we wanted to keep the option to work with old sparse versions. What do you suggest? Also, another question regarding dpdk-latest branch: I see the patches there above master are for adapting for latest dpdk. I have a series to offload frag match (introduced API for that in DPDK 20.11). Can I post it for dpdk-latest or wait for master to work with 20.11? > >>> Thanks >>> Ian >>> >>>> +#endif >>>> +#ifndef __atomic_store_n >>>> #define __atomic_store_n(p, val, memorder) (*(p) = (val)) >>>> +#endif >>>> >>>> /* Get actual <rte_mbuf.h> definitions for us to annotate and build on. */ >>>> #include_next <rte_mbuf.h> >>>> diff --git a/include/sparse/rte_trace_point.h b/include/sparse/rte_trace_point.h >>>> index c28f1c941..94bd54b25 100644 >>>> --- a/include/sparse/rte_trace_point.h >>>> +++ b/include/sparse/rte_trace_point.h >>>> @@ -18,8 +18,12 @@ >>>> #endif >>>> >>>> /* sparse doesn't know about gcc atomic builtins. */ >>>> +#ifndef __ATOMIC_ACQUIRE >>>> #define __ATOMIC_ACQUIRE 0 >>>> +#endif >>>> +#ifndef __atomic_load_n >>>> #define __atomic_load_n(p, memorder) *(p) >>>> +#endif >>>> >>>> /* Get actual <rte_trace_point.h> definitions for us to annotate and >>>> * build on. */ >>>> -- >>>> 2.28.0.546.g385c171
On Wed, Oct 21, 2020 at 3:53 PM Eli Britstein <elibr@nvidia.com> wrote: > >>> I see the changes to the ACQ_REL and ATOMIC_RELAXED, but no mention of the atomic_add_fetch, for my own curiosity must the fetch atomic fetch and gather operations also be wrapped because of the sparse patch or is this more an effort future proof and keep the definitions uniform in OVS? Same query WRT the atomic load operation below. > >> There is another recent patch: > >> https://git.kernel.org/pub/scm/devel/sparse/sparse.git/commit/?id=7cdf84691f33e4fc6a0198e1a04137993f3a37ad > >> This one introduces support for __atomic_add_fetch and friends. > >> I didn't test it, though. > > OTOH, __atomic_add_fetch is a builtin function, so, IIUC, ifndef will not work. > > I added ifndef as they have define in OVS. We can omit them at all as in > the previous revert series, but we wanted to keep the option to work > with old sparse versions. > > What do you suggest? For rte_mbuf.h: __ATOMIC_ACQ_REL __ATOMIC_RELAXED __atomic_add_fetch __atomic_store_n Added in sparse with: cf8f1047 - builtin: add predefines for __ATOMIC_RELAXED & friends (3 days ago) <Luc Van Oostenryck> 7cdf8469 - builtin: add support for __atomic_add_fetch(), ... (3 days ago) <Luc Van Oostenryck> f42e2afa - builtin: add support for others generic atomic builtins (3 days ago) <Luc Van Oostenryck> For rte_trace_point.h: __ATOMIC_ACQUIRE __atomic_load_n Added in sparse with: cf8f1047 - builtin: add predefines for __ATOMIC_RELAXED & friends (3 days ago) <Luc Van Oostenryck> f42e2afa - builtin: add support for others generic atomic builtins (3 days ago) <Luc Van Oostenryck> Checking for a single define is enough to protect all other defines, since it was an atomic patch. sparse reverting those builtins is unlikely and we can't detect them at the preprocessing step: I would move all of this under a single check on one of the defines.
diff --git a/include/sparse/rte_mbuf.h b/include/sparse/rte_mbuf.h index ee461f91e..172c9954a 100644 --- a/include/sparse/rte_mbuf.h +++ b/include/sparse/rte_mbuf.h @@ -18,10 +18,18 @@ #endif /* sparse doesn't know about gcc atomic builtins. */ +#ifndef __ATOMIC_ACQ_REL #define __ATOMIC_ACQ_REL 0 +#endif +#ifndef __ATOMIC_RELAXED #define __ATOMIC_RELAXED 1 +#endif +#ifndef __atomic_add_fetch #define __atomic_add_fetch(p, val, memorder) (*(p) = *(p) + (val)) +#endif +#ifndef __atomic_store_n #define __atomic_store_n(p, val, memorder) (*(p) = (val)) +#endif /* Get actual <rte_mbuf.h> definitions for us to annotate and build on. */ #include_next <rte_mbuf.h> diff --git a/include/sparse/rte_trace_point.h b/include/sparse/rte_trace_point.h index c28f1c941..94bd54b25 100644 --- a/include/sparse/rte_trace_point.h +++ b/include/sparse/rte_trace_point.h @@ -18,8 +18,12 @@ #endif /* sparse doesn't know about gcc atomic builtins. */ +#ifndef __ATOMIC_ACQUIRE #define __ATOMIC_ACQUIRE 0 +#endif +#ifndef __atomic_load_n #define __atomic_load_n(p, memorder) *(p) +#endif /* Get actual <rte_trace_point.h> definitions for us to annotate and * build on. */
In sparse commit [1], __ATOMIC_* defines were introduced, which cause redefinition errors. Wrap OVS defines with #ifndef to fix it. [1] https://git.kernel.org/pub/scm/devel/sparse/sparse.git/commit/?id=cf8f104749f5bca36852989297af8cc19ff24d5f Tested-at: https://travis-ci.org/github/elibritstein/OVS/builds/737660375 Signed-off-by: Eli Britstein <elibr@nvidia.com> --- include/sparse/rte_mbuf.h | 8 ++++++++ include/sparse/rte_trace_point.h | 4 ++++ 2 files changed, 12 insertions(+)