Re: [PATCH v4] lib/net: add MPLS insert and strip functionality

[Olivier Matz <olivier.matz@6wind.com>]

Hi,

Sorry for the late feedback, please see some comments below.

To be honest, I have some doubts about the usefulness of the patch,
especially the function that strips all the MPLS headers.

On Sat, Feb 25, 2023 at 06:53:43PM +0500, Tanzeel-inline wrote:
> From: Tanzeel Ahmed <tanxeel1.ahmed@gmail.com>
> 
> This patch is new version of [PATCH] lib/net: added push MPLS header API.
> I have also added the MPLS strip functionality to address the question
> asked in last patch.

You can remove this from commit log.
To add comments like these (which are indeed helpful), you can do it
after the '---', so it's not included in the commit log.

> 
> > You should explain why you add this function.

Please remove this line too.

> None of the foundational NICs currently supports MPLS insertion and
> stripping, this functionality can help users who rely on MPLS in their
> network application.
> 
> > I'm not sure it should be inline
> I did for performance in high-traffic application.

The inlining is probably discussable, but it is consistent with the
other equivalent functions in lib/net. Can you please reword the 2 lines
to remove the quote?

> 
> Signed-off-by: Tanzeel Ahmed <tanxeel1.ahmed@gmail.com>
> 
> ---
> v4:
> * Removed extra void cast.
> * rte_pktmbuf_append/mtod now return void*.
>   The memmove result is casted to rte_ether_hdr*.
> 
> v3:
> * fixed patch check failure issue
> 
> v2:
> * marked experimental
> * coding style fixed
> * changed rte_memcpy to memcpy
> * mpls header marked as const in parameter
> * added MPLS stripping functionality
> ---
>  .mailmap           |  1 +
>  lib/net/rte_mpls.h | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 98 insertions(+)
> 
> diff --git a/.mailmap b/.mailmap
> index a9f4f28..2af4e0d 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -1312,6 +1312,7 @@ Takeshi Yoshimura <tyos@jp.ibm.com> <t.yoshimura8869@gmail.com>
>  Takuya Asada <syuu@cloudius-systems.com>
>  Tal Avraham <talavr@annapurnalabs.com>
>  Tal Shnaiderman <talshn@nvidia.com> <talshn@mellanox.com>
> +Tanzeel Ahmed <tanxeel1.ahmed@gmail.com>
>  Tao Y Yang <tao.y.yang@intel.com>
>  Tao Zhu <taox.zhu@intel.com>
>  Taripin Samuel <samuel.taripin@intel.com>
> diff --git a/lib/net/rte_mpls.h b/lib/net/rte_mpls.h
> index 51523e7..d7e267f 100644
> --- a/lib/net/rte_mpls.h
> +++ b/lib/net/rte_mpls.h
> @@ -13,6 +13,8 @@
>  
>  #include <stdint.h>
>  #include <rte_byteorder.h>
> +#include <rte_ether.h>
> +#include <rte_mbuf.h>
>  
>  #ifdef __cplusplus
>  extern "C" {
> @@ -36,6 +38,101 @@ struct rte_mpls_hdr {
>  	uint8_t  ttl;       /**< Time to live. */
>  } __rte_packed;
>  
> +#define RTE_MPLS_HLEN 4 /**< Length of MPLS header. */
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Insert MPLS header into the packet.
> + * If it's first MPLS header to be inserted in the packet,
> + *  - Updates the ether type.
> + *  - Sets the MPLS bottom-of-stack bit to 1.

If it's first -> If it's the first

The first sentence uses the imperative form (which is preferred), so:
Updates -> Update
Sets -> Set

We can also document that the mbuf should not be shared, with enough
headroom.

> + *
> + * @param m
> + *   The pointer to the mbuf.
> + * @param mp
> + *   The pointer to the MPLS header.
> + * @return
> + *   0 on success, -1 on error

final dot at the end

> + */
> +__rte_experimental
> +static inline int
> +rte_mpls_push_over_l2(struct rte_mbuf **m, const struct rte_mpls_hdr *mp)

I don't see the need for **m, I think *m will do the job.

> +{
> +	void *os, *ns;
> +	struct rte_ether_hdr *nh;
> +	struct rte_mpls_hdr *mph;
> +
> +	/* Can't insert header if mbuf is shared */
> +	if (!RTE_MBUF_DIRECT(*m) || rte_mbuf_refcnt_read(*m) > 1)
> +		return -EINVAL;
> +
> +	/* Can't insert header if ethernet frame doesn't exist */
> +	if (rte_pktmbuf_data_len(*m) < RTE_ETHER_HDR_LEN)
> +		return -EINVAL;
> +
> +	os = rte_pktmbuf_mtod(*m, void *);

I prefer what you did in your previous revision (v3), without the *os
and *ns intermediate variables.

The first cast in (void *) is usually not needed in DPDK, but in your
case, it is better to add it because it is in a public header, and we
don't control the compiler warnings.

> +	ns = (void *)rte_pktmbuf_prepend(*m, sizeof(struct rte_mpls_hdr));
> +	if (ns == NULL)
> +		return -ENOSPC;
> +
> +	nh = (struct rte_ether_hdr *)memmove(ns, os, RTE_ETHER_HDR_LEN);
> +
> +	/* Copy the MPLS header after ethernet frame */
> +	mph = rte_pktmbuf_mtod_offset(*m, struct rte_mpls_hdr*,

"struct rte_mpls_hdr*" -> "struct rte_mpls_hdr *"

> +			sizeof(struct rte_ether_hdr));
> +	memcpy(mph, mp, RTE_MPLS_HLEN);
> +
> +	mph->tag_msb = rte_cpu_to_be_16(mp->tag_msb);
> +
> +	/* If first MPLS header, update ether type and bottom-of-stack bit */
> +	if (nh->ether_type != rte_cpu_to_be_16(RTE_ETHER_TYPE_MPLS)) {
> +		nh->ether_type = rte_cpu_to_be_16(RTE_ETHER_TYPE_MPLS);
> +		mph->bs = 1;
> +	} else {
> +		mph->bs = 0;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Strips MPLS from the packet. Doesn't update the ether type

-> Strip MPLS header from the packet, without updating the ether
   type.

I think the function should only strip the first MPLS header, it is
symmetric with the previous function, and more flexible.


> + *
> + * @param m
> + *   The pointer to the mbuf.
> + * @return
> + *   0 on success, -1 on error

final dot at the end

> + */
> +__rte_experimental
> +static inline int
> +rte_mpls_strip_over_l2(struct rte_mbuf *m)
> +{
> +	struct rte_ether_hdr *eh = rte_pktmbuf_mtod(m, struct rte_ether_hdr *);
> +	struct rte_mpls_hdr *mph;
> +	bool mpls_exist = true;
> +

We should check the packet length, and that the packet is not shared.

> +	if (eh->ether_type != rte_cpu_to_be_16(RTE_ETHER_TYPE_MPLS))
> +		return -1;
> +
> +	/* Stripping all MPLS header */
> +	while (mpls_exist) {
> +		mph = rte_pktmbuf_mtod_offset(m, struct rte_mpls_hdr*,

"struct rte_mpls_hdr*" -> "struct rte_mpls_hdr *"

> +		sizeof(struct rte_ether_hdr));

wrong indent

> +		if (mph->bs & 1)
> +			mpls_exist = false;
> +		memmove(rte_pktmbuf_adj(m, sizeof(struct rte_mpls_hdr)),
> +		eh, sizeof(struct rte_ether_hdr));

wrong indent

technically, it is similar, but I think it is more readable to have
memove(), then rte_pktmbuf_adj(), on 2 distinct lines.

> +		eh = rte_pktmbuf_mtod(m, struct rte_ether_hdr *);
> +	}
> +
> +	return 0;
> +}
> +
>  #ifdef __cplusplus
>  }
>  #endif
> -- 
> 1.8.3.1
> 

It would be great to have a unit test for these functions. This is even more
important because there is no user in the code (drivers or lib) that would
test it indirectly.