RE: [PATCH] net: fix IPv4 cksum simple function

["Morten Brørup" <mb@smartsharesystems.com>]

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Tuesday, 5 November 2024 10.28
> 
> On Tue, Nov 5, 2024 at 10:09 AM Morten Brørup
> <mb@smartsharesystems.com> wrote:
> > > diff --git a/lib/net/rte_ip4.h b/lib/net/rte_ip4.h
> > > index 4dd0058cc5..f9b8333332 100644
> > > --- a/lib/net/rte_ip4.h
> > > +++ b/lib/net/rte_ip4.h
> > > @@ -39,7 +39,7 @@ extern "C" {
> > >  /**
> > >   * IPv4 Header
> > >   */
> > > -struct rte_ipv4_hdr {
> > > +struct __rte_aligned(2) rte_ipv4_hdr {
> >
> > <unrelated>
> > I wonder why we have a convention of putting __rte_packed after the
> struct, and not between the "struct" tag and the name of the struct.
> > It would make the code much more readable having it here, like
> __rte_aligned().
> > </unrelated>
> 
> I agree that the previous convention was not great, as it has resulted
> in some funny jokes, like getting some __rte_XXX variables (in the
> absence of the right header inclusion defining __rte_XXX attribute
> macro).
> 
> __rte_aligned() "conventional" location has been changed recently by
> Tyler.
> __rte_packed is still conventionnally placed in a "legacy" position
> around the dpdk tree.
> It could probably be moved in the same way.
> 
> But there is still the question of packed structures with MSVC.
> Tyler proposal seemed to rely on the current __rte_packed conventional
> position.
> https://patchwork.dpdk.org/project/dpdk/patch/1713225913-20792-2-git-
> send-email-roretzla@linux.microsoft.com/
> Note that I am not a fan of this push/pop stuff.
> 
> Maybe Andre will find a better solution.

If we cannot come up with a clean solution that looks like an attribute (like GCC), we should accept MSVC's style with push/pop and learn to live with it.

Perhaps something like:

#ifdef RTE_TOOLCHAIN_MSVC
#define __RTE_PACKED(...) \
__pragma(pack(push, 1)) \
__VA_ARGS__ \
__pragma(pack(pop))
#else
#define __RTE_PACKED(...) __VA_ARGS__ __attribute__((__packed__))
#endif

This would also move the "packed" information to the top of the struct, making the code easier to read - i.e. easier to notice that the structure is packed when not hidden away at the end of the structure.

> 
> In any case, I prefer we keep __rte_packed position as is until this
> question is resolved.

Agree.
Better to change it in one pass, instead of two.