Re: [PATCH] lib/net: added push MPLS header API

[Stephen Hemminger <stephen@networkplumber.org>]

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.