From: Olivier Matz <olivier.matz@6wind.com>
To: Tanzeel-inline <tanxeel1.ahmed@gmail.com>
Cc: thomas@monjalon.net, tanzeelahmed713@gmail.com, dev@dpdk.org
Subject: Re: [PATCH v4] lib/net: add MPLS insert and strip functionality
Date: Mon, 5 Jun 2023 11:31:16 +0200 [thread overview]
Message-ID: <ZH2rZIXM2V7yJkqO@platinum> (raw)
In-Reply-To: <20230225135343.262-1-tanxeel1.ahmed@gmail.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.
next prev parent reply other threads:[~2023-06-05 9:31 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-19 22:43 [PATCH v2] " Tanzeel-inline
2023-02-24 11:25 ` [PATCH v3] " Tanzeel-inline
2023-02-24 16:29 ` Stephen Hemminger
2023-02-25 13:53 ` [PATCH v4] " Tanzeel-inline
2023-03-09 18:35 ` Tanzeel Ahmed
2023-04-18 13:18 ` Tanzeel Ahmed
2023-05-20 19:53 ` Tanzeel Ahmed
2023-06-05 9:31 ` Olivier Matz [this message]
2023-06-10 20:17 ` [PATCH v5] " Tanzeel-inline
2023-06-22 19:47 ` Tanzeel Ahmed
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZH2rZIXM2V7yJkqO@platinum \
--to=olivier.matz@6wind.com \
--cc=dev@dpdk.org \
--cc=tanxeel1.ahmed@gmail.com \
--cc=tanzeelahmed713@gmail.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).