[PATCH] net: fix IPv4 cksum simple function

[PATCH] net: fix IPv4 cksum simple function

[David Marchand]

The new function breaks compilation with -Wcast-align.

In file included from /home/runner/work/ovs/ovs/dpdk-dir/include/rte_ip.h:9:
/home/runner/work/ovs/ovs/dpdk-dir/include/rte_ip4.h:191:10:
	error: cast from 'const uint8_t *' (aka 'const unsigned char *')
	to 'const unaligned_uint16_t *' (aka 'const unsigned short *')
	increases required alignment from 1 to 2 [-Werror,-Wcast-align]
        v16_h = (const unaligned_uint16_t *)&ipv4_hdr->version_ihl;
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Fix this by aligning rte_ipv4_hdr to two bytes, and point at the start
of the structure rather than the first field (which happens to be 1 byte
large).

Fixes: f9e1d67f237a ("net: add IPv4 cksum function for simple cases")

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 doc/guides/rel_notes/release_24_11.rst | 2 ++
 lib/net/rte_ip4.h                      | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/doc/guides/rel_notes/release_24_11.rst b/doc/guides/rel_notes/release_24_11.rst
index 53a5ffebe5..d0152c558c 100644
--- a/doc/guides/rel_notes/release_24_11.rst
+++ b/doc/guides/rel_notes/release_24_11.rst
@@ -291,6 +291,8 @@ API Changes
     releases: it handles key=value and only-key cases.
   * Both ``rte_kvargs_process`` and ``rte_kvargs_process_opt`` reject a NULL ``kvlist`` parameter.
 
+* net: The IPv4 header structure ``rte_ipv4_hdr`` has been marked as two bytes aligned.
+
 * net: The ICMP message types ``RTE_IP_ICMP_ECHO_REPLY`` and ``RTE_IP_ICMP_ECHO_REQUEST``
   are marked as deprecated, and are replaced
   by ``RTE_ICMP_TYPE_ECHO_REPLY`` and ``RTE_ICMP_TYPE_ECHO_REQUEST``.
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 {
 	__extension__
 	union {
 		uint8_t version_ihl;    /**< version and header length */
@@ -188,7 +188,7 @@ rte_ipv4_cksum_simple(const struct rte_ipv4_hdr *ipv4_hdr)
 	 * Compute the sum of successive 16-bit words of the IPv4 header,
 	 * skipping the checksum field of the header.
 	 */
-	v16_h = (const unaligned_uint16_t *)&ipv4_hdr->version_ihl;
+	v16_h = (const uint16_t *)ipv4_hdr;
 	ip_cksum = v16_h[0] + v16_h[1] + v16_h[2] + v16_h[3] +
 		v16_h[4] + v16_h[6] + v16_h[7] + v16_h[8] + v16_h[9];
 
-- 
2.46.2

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

[Morten Brørup]

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Tuesday, 5 November 2024 09.59
> 
> The new function breaks compilation with -Wcast-align.
> 
> In file included from /home/runner/work/ovs/ovs/dpdk-
> dir/include/rte_ip.h:9:
> /home/runner/work/ovs/ovs/dpdk-dir/include/rte_ip4.h:191:10:
> 	error: cast from 'const uint8_t *' (aka 'const unsigned char *')
> 	to 'const unaligned_uint16_t *' (aka 'const unsigned short *')
> 	increases required alignment from 1 to 2 [-Werror,-Wcast-align]
>         v16_h = (const unaligned_uint16_t *)&ipv4_hdr->version_ihl;
>                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Fix this by aligning rte_ipv4_hdr to two bytes, and point at the start
> of the structure rather than the first field (which happens to be 1
> byte
> large).
> 
> Fixes: f9e1d67f237a ("net: add IPv4 cksum function for simple cases")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  doc/guides/rel_notes/release_24_11.rst | 2 ++
>  lib/net/rte_ip4.h                      | 4 ++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/release_24_11.rst
> b/doc/guides/rel_notes/release_24_11.rst
> index 53a5ffebe5..d0152c558c 100644
> --- a/doc/guides/rel_notes/release_24_11.rst
> +++ b/doc/guides/rel_notes/release_24_11.rst
> @@ -291,6 +291,8 @@ API Changes
>      releases: it handles key=value and only-key cases.
>    * Both ``rte_kvargs_process`` and ``rte_kvargs_process_opt`` reject
> a NULL ``kvlist`` parameter.
> 
> +* net: The IPv4 header structure ``rte_ipv4_hdr`` has been marked as
> two bytes aligned.
> +
>  * net: The ICMP message types ``RTE_IP_ICMP_ECHO_REPLY`` and
> ``RTE_IP_ICMP_ECHO_REQUEST``
>    are marked as deprecated, and are replaced
>    by ``RTE_ICMP_TYPE_ECHO_REPLY`` and ``RTE_ICMP_TYPE_ECHO_REQUEST``.
> 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>

>  	__extension__
>  	union {
>  		uint8_t version_ihl;    /**< version and header length */
> @@ -188,7 +188,7 @@ rte_ipv4_cksum_simple(const struct rte_ipv4_hdr
> *ipv4_hdr)
>  	 * Compute the sum of successive 16-bit words of the IPv4 header,
>  	 * skipping the checksum field of the header.
>  	 */
> -	v16_h = (const unaligned_uint16_t *)&ipv4_hdr->version_ihl;
> +	v16_h = (const uint16_t *)ipv4_hdr;
>  	ip_cksum = v16_h[0] + v16_h[1] + v16_h[2] + v16_h[3] +
>  		v16_h[4] + v16_h[6] + v16_h[7] + v16_h[8] + v16_h[9];
> 
> --
> 2.46.2

Reviewed-by: Morten Brørup <mb@smartsharesystems.com>

For consistency, could one of you - David or Robin - please also 2-byte align the IPv6 header structure?

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

[David Marchand]

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.

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


-- 
David Marchand

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

[Robin Jarry]

Morten Brørup, Nov 05, 2024 at 10:09:
> For consistency, could one of you - David or Robin - please also 
> 2-byte align the IPv6 header structure?

I can send a patch but I wonder if this is really necessary after the 
RC-1 has passed?

We don't have any gcc warning related to IPv6 unaligned access (except 
maybe in drivers).

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

[Morten Brørup]

> From: Robin Jarry [mailto:rjarry@redhat.com]
> Sent: Tuesday, 5 November 2024 10.37
> 
> Morten Brørup, Nov 05, 2024 at 10:09:
> > For consistency, could one of you - David or Robin - please also
> > 2-byte align the IPv6 header structure?
> 
> I can send a patch but I wonder if this is really necessary after the
> RC-1 has passed?

Yes, please.
And don't forget the extension headers (rte_ipv6_routing_ext and rte_ipv6_fragment_ext).

> 
> We don't have any gcc warning related to IPv6 unaligned access (except
> maybe in drivers).

I think both IPv4 and IPv6 headers are always 2-byte aligned IRL; so for consistency and to prevent potential future problems, the API should reflect this.

Consistency is important: If the alignment differs between IPv4 and IPv6 headers in the API, someone might think there is a good reason for this, and as time passes no one will be able to answer why there is a difference.

PS:
I am also pushing for 2-byte aligning Layer 4 headers (TCP, UDP, etc.), e.g. for checksumming purposes; but didn't get enough response to that RFC:
https://inbox.dpdk.org/dev/20241011160653.88028-1-mb@smartsharesystems.com/

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

[Bruce Richardson]

On Tue, Nov 05, 2024 at 09:59:11AM +0100, David Marchand wrote:
> The new function breaks compilation with -Wcast-align.
> 
> In file included from /home/runner/work/ovs/ovs/dpdk-dir/include/rte_ip.h:9:
> /home/runner/work/ovs/ovs/dpdk-dir/include/rte_ip4.h:191:10:
> 	error: cast from 'const uint8_t *' (aka 'const unsigned char *')
> 	to 'const unaligned_uint16_t *' (aka 'const unsigned short *')
> 	increases required alignment from 1 to 2 [-Werror,-Wcast-align]
>         v16_h = (const unaligned_uint16_t *)&ipv4_hdr->version_ihl;
>                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Fix this by aligning rte_ipv4_hdr to two bytes, and point at the start
> of the structure rather than the first field (which happens to be 1 byte
> large).
> 
> Fixes: f9e1d67f237a ("net: add IPv4 cksum function for simple cases")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  doc/guides/rel_notes/release_24_11.rst | 2 ++
>  lib/net/rte_ip4.h                      | 4 ++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

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

[Morten Brørup]

> 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.

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

[David Marchand]

On Tue, Nov 5, 2024 at 11:20 AM Morten Brørup <mb@smartsharesystems.com> wrote:
> > 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.

Well, there is probably not many solutions.
OVS does the same as what you suggest.


>
> 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.

__RTE_PACKED(struct __rte_aligned(2) rte_ipv4_hdr {
...
});

Agreed, looks better.


-- 
David Marchand

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

[Bruce Richardson]

On Tue, Nov 05, 2024 at 11:49:52AM +0100, David Marchand wrote:
> On Tue, Nov 5, 2024 at 11:20 AM Morten Brørup <mb@smartsharesystems.com> wrote:
> > > 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.
> 
> Well, there is probably not many solutions.
> OVS does the same as what you suggest.
> 
> 
> >
> > 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.
> 
> __RTE_PACKED(struct __rte_aligned(2) rte_ipv4_hdr {
> ...
> });
> 
> Agreed, looks better.
> 
Not convinced it looks better myself. Rather than packing the structure,
can we instead put aligned(2) on the 32-bit fields inside it?

/Bruce

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

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Tuesday, 5 November 2024 12.03
> 
> On Tue, Nov 05, 2024 at 11:49:52AM +0100, David Marchand wrote:
> > On Tue, Nov 5, 2024 at 11:20 AM Morten Brørup
> <mb@smartsharesystems.com> wrote:
> > > > 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.
> >
> > Well, there is probably not many solutions.
> > OVS does the same as what you suggest.
> >
> >
> > >
> > > 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.
> >
> > __RTE_PACKED(struct __rte_aligned(2) rte_ipv4_hdr {
> > ...
> > });
> >
> > Agreed, looks better.
> >
> Not convinced it looks better myself. Rather than packing the
> structure,
> can we instead put aligned(2) on the 32-bit fields inside it?

No, MSVC aligned() can only increase alignment, not decrease it.
I checked it a few minutes ago, getting the same idea. ;-)

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

[David Marchand]

On Tue, Nov 5, 2024 at 12:08 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Tue, Nov 05, 2024 at 11:49:52AM +0100, David Marchand wrote:
> > On Tue, Nov 5, 2024 at 11:20 AM Morten Brørup <mb@smartsharesystems.com> wrote:
> > > > 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.
> >
> > Well, there is probably not many solutions.
> > OVS does the same as what you suggest.
> >
> >
> > >
> > > 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.
> >
> > __RTE_PACKED(struct __rte_aligned(2) rte_ipv4_hdr {
> > ...
> > });
> >
> > Agreed, looks better.
> >
> Not convinced it looks better myself. Rather than packing the structure,
> can we instead put aligned(2) on the 32-bit fields inside it?

Marking a field won't have an effect, unless packing the structure:
https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html

When used on a struct, or struct member, the aligned attribute can
only increase the alignment; in order to decrease it, the packed
attribute must be specified as well. When used as part of a typedef,
the aligned attribute can both increase and decrease alignment, and
specifying the packed attribute generates a warning.


-- 
David Marchand

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

[David Marchand]

On Tue, Nov 5, 2024 at 10:09 AM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: David Marchand [mailto:david.marchand@redhat.com]
> > Sent: Tuesday, 5 November 2024 09.59
> >
> > The new function breaks compilation with -Wcast-align.
> >
> > In file included from /home/runner/work/ovs/ovs/dpdk-
> > dir/include/rte_ip.h:9:
> > /home/runner/work/ovs/ovs/dpdk-dir/include/rte_ip4.h:191:10:
> >       error: cast from 'const uint8_t *' (aka 'const unsigned char *')
> >       to 'const unaligned_uint16_t *' (aka 'const unsigned short *')
> >       increases required alignment from 1 to 2 [-Werror,-Wcast-align]
> >         v16_h = (const unaligned_uint16_t *)&ipv4_hdr->version_ihl;
> >                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Fix this by aligning rte_ipv4_hdr to two bytes, and point at the start
> > of the structure rather than the first field (which happens to be 1
> > byte
> > large).
> >
> > Fixes: f9e1d67f237a ("net: add IPv4 cksum function for simple cases")
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>

Applied, thanks.


-- 
David Marchand