[ovs-dev,dpdk-latest,v1] build: Fix build with Sparse for rte_mbuf.h.

Introduction of C11 atomic instructions in rte_mbuf.h causes the build
to fail with Sparse reporting following errors.

error: undefined identifier '__atomic_add_fetch'
error: undefined identifier '__atomic_store_n'

This patch udpates the Sprase headers for rte_mbuf.h.

Tested-at: https://travis-ci.org/github/Sunil-Pai-G/ovs-copy/builds/723223426
Signed-off-by: Sunil Pai G <sunil.pai.g@intel.com>
---
 include/sparse/automake.mk |  1 +
 include/sparse/rte_mbuf.h  | 27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)
 create mode 100644 include/sparse/rte_mbuf.h

Thanks for fixing.

Afaics in OVS, patches touching sparse headers have a sparse: prefix.
+ my personal preference is not to mention actual filenames in a title.

How about the following title?
sparse: Fix build with DPDK 20.08.

On Wed, Sep 2, 2020 at 3:56 PM Sunil Pai G <sunil.pai.g@intel.com> wrote:
>
> Introduction of C11 atomic instructions in rte_mbuf.h causes the build
> to fail with Sparse reporting following errors.
>
> error: undefined identifier '__atomic_add_fetch'
> error: undefined identifier '__atomic_store_n'
>
> This patch udpates the Sprase headers for rte_mbuf.h.

typo updates* Sparse*, but I'd rather say that we add a sparse header
(there was no rte_mbuf.h wrapper so far).


>
> Tested-at: https://travis-ci.org/github/Sunil-Pai-G/ovs-copy/builds/723223426
> Signed-off-by: Sunil Pai G <sunil.pai.g@intel.com>
> ---
>  include/sparse/automake.mk |  1 +
>  include/sparse/rte_mbuf.h  | 27 +++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
>  create mode 100644 include/sparse/rte_mbuf.h
>
> diff --git a/include/sparse/automake.mk b/include/sparse/automake.mk
> index c6ced9387..2c1ef68fc 100644
> --- a/include/sparse/automake.mk
> +++ b/include/sparse/automake.mk
> @@ -11,6 +11,7 @@ noinst_HEADERS += \
>          include/sparse/netpacket/packet.h \
>          include/sparse/pthread.h \
>          include/sparse/rte_atomic.h \
> +       include/sparse/rte_mbuf.h \

Indentation is not the same as the rest of this file.


>          include/sparse/rte_memcpy.h \
>          include/sparse/rte_trace_point.h \
>          include/sparse/sys/socket.h \
> diff --git a/include/sparse/rte_mbuf.h b/include/sparse/rte_mbuf.h
> new file mode 100644
> index 000000000..61b53279f
> --- /dev/null
> +++ b/include/sparse/rte_mbuf.h
> @@ -0,0 +1,27 @@
> +/* Copyright (c) 2020 Intel, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#ifndef __CHECKER__
> +#error "Use this header only with sparse.  It is not a correct implementation."
> +#endif
> +
> +/* sparse doesn't know about gcc atomic builtins. */
> +#define __ATOMIC_ACQ_REL 0
> +#define __ATOMIC_RELAXED 1
> +#define __atomic_add_fetch(a, b, c) ((*a)=(*a)+b)
> +#define __atomic_store_n(a, b, c) (*a=b)

a, b, c give no hint at what they refer to.
I'd rather use variable names like p, val and memorder.
Afaics in OVS, there is nothing preventing us from using spaces in macros.
And I may be paranoid but I would protect what is passed through those macros.

So how about something like:
+#define __atomic_add_fetch(p, val, memorder) (*(p) = *(p) + (val))
+#define __atomic_store_n(p, val, memorder) (*(p) = (val))


> +
> +/* Get actual <rte_mbuf.h> definitions for us to annotate and build on. */
> +#include_next <rte_mbuf.h>

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, September 3, 2020 1:11 PM
> To: Pai G, Sunil <sunil.pai.g@intel.com>
> Cc: ovs dev <dev@openvswitch.org>; Ilya Maximets <i.maximets@ovn.org>;
> Stokes, Ian <ian.stokes@intel.com>
> Subject: Re: [ovs-dev] [PATCH dpdk-latest v1] build: Fix build with Sparse for
> rte_mbuf.h.
> 
> Thanks for fixing.
> 
> Afaics in OVS, patches touching sparse headers have a sparse: prefix.
> + my personal preference is not to mention actual filenames in a title.
> 
> How about the following title?
> sparse: Fix build with DPDK 20.08.

Sure , will address this in the next version.

> 
> On Wed, Sep 2, 2020 at 3:56 PM Sunil Pai G <sunil.pai.g@intel.com> wrote:
> >
> > Introduction of C11 atomic instructions in rte_mbuf.h causes the build
> > to fail with Sparse reporting following errors.
> >
> > error: undefined identifier '__atomic_add_fetch'
> > error: undefined identifier '__atomic_store_n'
> >
> > This patch udpates the Sprase headers for rte_mbuf.h.
> 
> typo updates* Sparse*, but I'd rather say that we add a sparse header (there was
> no rte_mbuf.h wrapper so far).
> 

Agreed , shall address this .

> 
> >
> > Tested-at:
> > https://travis-ci.org/github/Sunil-Pai-G/ovs-copy/builds/723223426
> > Signed-off-by: Sunil Pai G <sunil.pai.g@intel.com>
> > ---
> >  include/sparse/automake.mk |  1 +
> >  include/sparse/rte_mbuf.h  | 27 +++++++++++++++++++++++++++
> >  2 files changed, 28 insertions(+)
> >  create mode 100644 include/sparse/rte_mbuf.h
> >
> > diff --git a/include/sparse/automake.mk b/include/sparse/automake.mk
> > index c6ced9387..2c1ef68fc 100644
> > --- a/include/sparse/automake.mk
> > +++ b/include/sparse/automake.mk
> > @@ -11,6 +11,7 @@ noinst_HEADERS += \
> >          include/sparse/netpacket/packet.h \
> >          include/sparse/pthread.h \
> >          include/sparse/rte_atomic.h \
> > +       include/sparse/rte_mbuf.h \
> 
> Indentation is not the same as the rest of this file.

Will address this in next version.

> 
> 
> >          include/sparse/rte_memcpy.h \
> >          include/sparse/rte_trace_point.h \
> >          include/sparse/sys/socket.h \ diff --git
> > a/include/sparse/rte_mbuf.h b/include/sparse/rte_mbuf.h new file mode
> > 100644 index 000000000..61b53279f
> > --- /dev/null
> > +++ b/include/sparse/rte_mbuf.h
> > @@ -0,0 +1,27 @@
> > +/* Copyright (c) 2020 Intel, Inc.
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + *     http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing,
> > +software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> implied.
> > + * See the License for the specific language governing permissions
> > +and
> > + * limitations under the License.
> > + */
> > +
> > +#ifndef __CHECKER__
> > +#error "Use this header only with sparse.  It is not a correct implementation."
> > +#endif
> > +
> > +/* sparse doesn't know about gcc atomic builtins. */ #define
> > +__ATOMIC_ACQ_REL 0 #define __ATOMIC_RELAXED 1 #define
> > +__atomic_add_fetch(a, b, c) ((*a)=(*a)+b) #define __atomic_store_n(a,
> > +b, c) (*a=b)
> 
> a, b, c give no hint at what they refer to.
> I'd rather use variable names like p, val and memorder.
> Afaics in OVS, there is nothing preventing us from using spaces in macros.
> And I may be paranoid but I would protect what is passed through those macros.
> 
> So how about something like:
> +#define __atomic_add_fetch(p, val, memorder) (*(p) = *(p) + (val))
> +#define __atomic_store_n(p, val, memorder) (*(p) = (val))

Agreed. Looks better. Will address this.

> 
> 
> > +
> > +/* Get actual <rte_mbuf.h> definitions for us to annotate and build
> > +on. */ #include_next <rte_mbuf.h>
> 
> 
> --
> David Marchand
Thanks and regards,
Sunil