From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 1DB4942C34; Mon, 5 Jun 2023 11:31:19 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D681240ED6; Mon, 5 Jun 2023 11:31:18 +0200 (CEST) Received: from mail-wr1-f53.google.com (mail-wr1-f53.google.com [209.85.221.53]) by mails.dpdk.org (Postfix) with ESMTP id 86D8440ED6 for ; Mon, 5 Jun 2023 11:31:17 +0200 (CEST) Received: by mail-wr1-f53.google.com with SMTP id ffacd0b85a97d-30e407daab5so1022250f8f.0 for ; Mon, 05 Jun 2023 02:31:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; t=1685957477; x=1688549477; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=oy6X2ko0ylff/KAa6YLUjGYfx09AEdkwVSTUuj2mAfc=; b=LEHpFmggsJPVmq1pwDBSum7HKQtZf1gl2BUQOZXWZWdrv313N2LfSU4UJ2R9ruVqHy 6Nf/SFGwOEs7vfrJiLbVDhsUzZFXtnxDymppKiPj746mkJXwHI8jLhyfV+xsHACEhpdI /y2pzUgVjeC8PvFHSv/KXHEUEWDqHzqDt4e4Ol4iuPDL9yYUAZv7IJle3M3Cff2spiZ9 BPbmnMtqqSZAthFe4ufDoTrxomRLvfzSot8AMRGV7qsidx3zKctQaKgwhvvVs6scH2R0 dnRs7s3w3GhmKOGeNrgcC+586Ugoz3KU7lz900xnp+yB+MNSOX5tPhSIvR0KpRewPvaC AscA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685957477; x=1688549477; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=oy6X2ko0ylff/KAa6YLUjGYfx09AEdkwVSTUuj2mAfc=; b=gzVOjDEVpfbfj3WJ5AM4hbnlXgWd4Aj5Vq/UukvDUXx87pVw1HwXTYoCYylAE+DYk/ ZyYaVucJEYmMQ4Jh6BtpyfYS3yBdGUZ0oDQj8nxCd0gXOru3wUoOO90i7HFVWMmCBmJj qPLMODTE2bsqoYcLHZ0jAaFNkPhB8fdydSStL5dzKVtQnavbm80J4oHF3mCeqNlc4kmH CIFPz95uE/d3bOFQKZHX9r/dLfUPW5EJ3D9sB1dft6c1ptwEVVOA3TA48fYXN22wE7kE lBm1GUcmyGhp5wDA5ra0/CFPSr9ja/BQ8IgK6dwfHfzYioMU0l5d4/eist7WvX7h/lKi v9+w== X-Gm-Message-State: AC+VfDwqv7f5CeMwE6qcsLm+QUUEy5qlc9Q+S7pkgNNGYdiS9JpGI8Ol U3aED0Sx5gxXBnHllWNxEQflKw== X-Google-Smtp-Source: ACHHUZ7nXiOnxNXJQ8/vhLQDRWp3Pt2nJ9E9SOUFo7SqRq15BEq6QQ65NjeMFk6s/4MzRfAgcz1TQA== X-Received: by 2002:a5d:4243:0:b0:30d:981d:a04a with SMTP id s3-20020a5d4243000000b0030d981da04amr5506754wrr.1.1685957477120; Mon, 05 Jun 2023 02:31:17 -0700 (PDT) Received: from 6wind.com ([2a01:e0a:5ac:6460:c065:401d:87eb:9b25]) by smtp.gmail.com with ESMTPSA id q10-20020a5d574a000000b003047d5b8817sm9257799wrw.80.2023.06.05.02.31.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 05 Jun 2023 02:31:16 -0700 (PDT) Date: Mon, 5 Jun 2023 11:31:16 +0200 From: Olivier Matz To: Tanzeel-inline Cc: thomas@monjalon.net, tanzeelahmed713@gmail.com, dev@dpdk.org Subject: Re: [PATCH v4] lib/net: add MPLS insert and strip functionality Message-ID: References: <20230219224334.309-1-tanxeel1.ahmed@gmail.com> <20230225135343.262-1-tanxeel1.ahmed@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230225135343.262-1-tanxeel1.ahmed@gmail.com> X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 > > 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 > > --- > 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 > Takuya Asada > Tal Avraham > Tal Shnaiderman > +Tanzeel Ahmed > Tao Y Yang > Tao Zhu > Taripin Samuel > 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 > #include > +#include > +#include > > #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.