From: Stephen Hemminger <stephen@networkplumber.org>
To: Tanzeel-inline <tanxeel1.ahmed@gmail.com>
Cc: olivier.matz@6wind.com, dev@dpdk.org,
Tanzeel Ahmed <tanzeelahmed713@gmail.com>
Subject: Re: [PATCH] lib/net: added push MPLS header API
Date: Sat, 17 Dec 2022 11:46:27 -0800 [thread overview]
Message-ID: <20221217114627.49cdb115@hermes.local> (raw)
In-Reply-To: <1671299997-13001-1-git-send-email-tanxeel1.ahmed@gmail.com>
On Sat, 17 Dec 2022 12:59:57 -0500
Tanzeel-inline <tanxeel1.ahmed@gmail.com> wrote:
> +/**
> + * 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.
> + *
> + * @param m
> + * The pointer to the mbuf.
> + * @param mp
> + * The pointer to the MPLS header.
> + * @return
> + * 0 on success, -1 on error (If no ethernet header exists)
> + */
Should be marked experimental.
> +static inline int
> +rte_mpls_push_over_l2(struct rte_mbuf **m, struct rte_mpls_hdr *mp)
trailing blank?
can mpls hdr be const?
> +{
> + struct rte_ether_hdr *oh, *nh;
> +
> + /* 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*/
add space after /* and before */
> + if (rte_pktmbuf_data_len(*m) < RTE_ETHER_HDR_LEN)
> + return -EINVAL;
> +
> + oh = rte_pktmbuf_mtod(*m, struct rte_ether_hdr *);
> + nh = (struct rte_ether_hdr *)(void *)
> + rte_pktmbuf_prepend(*m, sizeof(struct rte_mpls_hdr));
Not your problem, but having to double cast is only an artifact of
the poor design choice in DPDK for the return type of rte_pkmtmbuf_prepend()
> + if (nh == NULL)
> + return -ENOSPC;
> +
> + memmove(nh, oh, RTE_ETHER_HDR_LEN);
> +
> + mp->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))
> + {
DPDK like kernel uses bracket "cuddle" style.
if (conditon {
...
} else {
}
> + nh->ether_type = rte_cpu_to_be_16(RTE_ETHER_TYPE_MPLS);
> + mp->bs = 1;
> + }
> + else
> + {
> + mp->bs = 0;
> + }
> +
> + /* Copy the MPLS header after ethernet frame */
> + rte_memcpy(rte_pktmbuf_mtod_offset(*m, char*, sizeof(struct rte_ether_hdr)), mp, RTE_MPLS_HLEN);
rte_memcpy is not preferred for small values. Compiler can optimize memcpy() of small const sizes.
Might even be better to use structure assignment?
> +
> + (*m)->data_len += RTE_MPLS_HLEN;
> + return 0;
> +}
> +
> #ifdef __cplusplus
> }
> #endif
Side note: Why is DPDK version of checkpatch ignoring things that kernel checkpatch does not?
$ ~/kernel/linux//scripts/checkpatch.pl /tmp/mpls.mbox
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#102:
Push MPLS header after ethernet header, updates ethernet type and MPLS bs bit if required.
ERROR: trailing whitespace
#143: FILE: lib/net/rte_mpls.h:45:
+ * $
ERROR: trailing whitespace
#147: FILE: lib/net/rte_mpls.h:49:
+ * $
ERROR: trailing whitespace
#153: FILE: lib/net/rte_mpls.h:55:
+ * 0 on success, -1 on error (If no ethernet header exists)^I^I$
ERROR: trailing whitespace
#156: FILE: lib/net/rte_mpls.h:58:
+rte_mpls_push_over_l2(struct rte_mbuf **m, struct rte_mpls_hdr *mp) $
ERROR: trailing whitespace
#177: FILE: lib/net/rte_mpls.h:79:
+^I$
ERROR: that open brace { should be on the previous line
#179: FILE: lib/net/rte_mpls.h:81:
+ if (nh->ether_type != rte_cpu_to_be_16(RTE_ETHER_TYPE_MPLS))
+ {
ERROR: trailing whitespace
#183: FILE: lib/net/rte_mpls.h:85:
+^I}^I$
ERROR: that open brace { should be on the previous line
#184: FILE: lib/net/rte_mpls.h:86:
+ else
+ {
ERROR: else should follow close brace '}'
#184: FILE: lib/net/rte_mpls.h:86:
+ }
+ else
WARNING: line length of 104 exceeds 100 columns
#190: FILE: lib/net/rte_mpls.h:92:
+ rte_memcpy(rte_pktmbuf_mtod_offset(*m, char*, sizeof(struct rte_ether_hdr)), mp, RTE_MPLS_HLEN);
ERROR: trailing whitespace
#191: FILE: lib/net/rte_mpls.h:93:
+^I$
ERROR: Missing Signed-off-by: line by nominal patch author 'Tanzeel-inline <tanxeel1.ahmed@gmail.com>'
total: 11 errors, 2 warnings, 78 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
NOTE: Whitespace errors detected.
You may wish to use scripts/cleanpatch or scripts/cleanfile
/tmp/mpls.mbox has style problems, please review.
Also, current DPDK checkpatch has issue with mailmap.
This is not your problem. Looks like the change to check for mailmap
has chicken/egg problem
Tanzeel Ahmed <tanzeelahmed713@gmail.com> is unknown, please fix the commit message or update .mailmap.
prev parent reply other threads:[~2022-12-17 19:46 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-17 17:59 Tanzeel-inline
2022-12-17 19:46 ` Stephen Hemminger [this message]
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=20221217114627.49cdb115@hermes.local \
--to=stephen@networkplumber.org \
--cc=dev@dpdk.org \
--cc=olivier.matz@6wind.com \
--cc=tanxeel1.ahmed@gmail.com \
--cc=tanzeelahmed713@gmail.com \
/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).