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

[Pai G, Sunil]

> -----Original Message-----
> From: David Marchand <david.marchand at redhat.com>
> Sent: Thursday, September 3, 2020 1:11 PM
> To: Pai G, Sunil <sunil.pai.g at intel.com>
> Cc: ovs dev <dev at openvswitch.org>; Ilya Maximets <i.maximets at ovn.org>;
> Stokes, Ian <ian.stokes at 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 at 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 at 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