Re: [dpdk-dev] [PATCH v2 09/15] net/vmxnet3: switch MSS hint to dynamic mbuf field

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

On Mon, Oct 26, 2020 at 11:20:07PM +0100, Thomas Monjalon wrote:
> The segment count, used for MSS guess,
> was stored in the deprecated mbuf field udata64.
> It is moved to a dynamic field in order to allow removal of udata64.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

After seeing this commit, I wonder if we shouldn't introduce an
enhancement in the dynamic field API.

Previously, the driver used udata64 only internally, so without any
risk. The risky usages of udata64 were when the mbuf goes out of a
module.

Changing to dynamic field makes the code safe for any use, but consumes
more memory.

I wonder if we shouldn't (later) introduce a flag RTE_MBUF_DYN_F_SHARED,
or something similar, to say that this field is only used inside a
module, and that its memory can be shared with other dynamic fields.


> ---
>  drivers/net/vmxnet3/vmxnet3_ethdev.c | 15 +++++++++++++++
>  drivers/net/vmxnet3/vmxnet3_ethdev.h |  6 ++++++
>  drivers/net/vmxnet3/vmxnet3_rxtx.c   |  9 +++++----
>  3 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> index 6920ab568c..6e99b67b5a 100644
> --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
> +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> @@ -59,6 +59,8 @@
>  	 DEV_RX_OFFLOAD_JUMBO_FRAME |   \
>  	 DEV_RX_OFFLOAD_RSS_HASH)
>  
> +int vmxnet3_segs_dynfield_offset;
> +
>  static int eth_vmxnet3_dev_init(struct rte_eth_dev *eth_dev);
>  static int eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev);
>  static int vmxnet3_dev_configure(struct rte_eth_dev *dev);
> @@ -233,6 +235,11 @@ eth_vmxnet3_dev_init(struct rte_eth_dev *eth_dev)
>  	struct vmxnet3_hw *hw = eth_dev->data->dev_private;
>  	uint32_t mac_hi, mac_lo, ver;
>  	struct rte_eth_link link;
> +	static const struct rte_mbuf_dynfield vmxnet3_segs_dynfield_desc = {
> +		.name = VMXNET3_SEGS_DYNFIELD_NAME,
> +		.size = sizeof(VMXNET3_SEGS_DYNFIELD_TYPE),
> +		.align = __alignof__(VMXNET3_SEGS_DYNFIELD_TYPE),
> +	};
>  
>  	PMD_INIT_FUNC_TRACE();
>  
> @@ -242,6 +249,14 @@ eth_vmxnet3_dev_init(struct rte_eth_dev *eth_dev)
>  	eth_dev->tx_pkt_prepare = vmxnet3_prep_pkts;
>  	pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
>  
> +	/* extra mbuf field is required to guess MSS */
> +	vmxnet3_segs_dynfield_offset =
> +		rte_mbuf_dynfield_register(&vmxnet3_segs_dynfield_desc);
> +	if (vmxnet3_segs_dynfield_offset < 0) {
> +		PMD_INIT_LOG(ERR, "Cannot register mbuf field.");
> +		return -rte_errno;
> +	}
> +
>  	/*
>  	 * for secondary processes, we don't initialize any further as primary
>  	 * has already done this work.
> diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.h b/drivers/net/vmxnet3/vmxnet3_ethdev.h
> index dd685b02b7..e35edf07be 100644
> --- a/drivers/net/vmxnet3/vmxnet3_ethdev.h
> +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.h
> @@ -193,4 +193,10 @@ uint16_t vmxnet3_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
>  uint16_t vmxnet3_prep_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
>  			uint16_t nb_pkts);
>  
> +extern int vmxnet3_segs_dynfield_offset;
> +#define VMXNET3_SEGS_DYNFIELD_NAME "rte_net_vmxnet3_dynfield_segs"
> +#define VMXNET3_SEGS_DYNFIELD_TYPE uint8_t
> +#define VMXNET3_SEGS_DYNFIELD(mbuf) (*RTE_MBUF_DYNFIELD(rxm, \
> +		vmxnet3_segs_dynfield_offset, VMXNET3_SEGS_DYNFIELD_TYPE *))
> +
>  #endif /* _VMXNET3_ETHDEV_H_ */
> diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c b/drivers/net/vmxnet3/vmxnet3_rxtx.c
> index e10f9ee870..2b0e2e6f19 100644
> --- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
> +++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
> @@ -674,6 +674,7 @@ vmxnet3_guess_mss(struct vmxnet3_hw *hw, const Vmxnet3_RxCompDesc *rcd,
>  	struct rte_ipv6_hdr *ipv6_hdr;
>  	struct rte_tcp_hdr *tcp_hdr;
>  	char *ptr;
> +	uint8_t segs;
>  
>  	RTE_ASSERT(rcd->tcp);
>  
> @@ -710,9 +711,9 @@ vmxnet3_guess_mss(struct vmxnet3_hw *hw, const Vmxnet3_RxCompDesc *rcd,
>  	tcp_hdr = (struct rte_tcp_hdr *)(ptr + hlen);
>  	hlen += (tcp_hdr->data_off & 0xf0) >> 2;
>  
> -	if (rxm->udata64 > 1)
> -		return (rte_pktmbuf_pkt_len(rxm) - hlen +
> -				rxm->udata64 - 1) / rxm->udata64;
> +	segs = VMXNET3_SEGS_DYNFIELD(rxm);
> +	if (segs > 1)
> +		return (rte_pktmbuf_pkt_len(rxm) - hlen + segs - 1) / segs;
>  	else
>  		return hw->mtu - hlen + sizeof(struct rte_ether_hdr);
>  }
> @@ -737,7 +738,7 @@ vmxnet3_rx_offload(struct vmxnet3_hw *hw, const Vmxnet3_RxCompDesc *rcd,
>  					(const Vmxnet3_RxCompDescExt *)rcd;
>  
>  			rxm->tso_segsz = rcde->mss;
> -			rxm->udata64 = rcde->segCnt;
> +			VMXNET3_SEGS_DYNFIELD(rxm) = rcde->segCnt;
>  			ol_flags |= PKT_RX_LRO;
>  		}
>  	} else { /* Offloads set in eop */
> -- 
> 2.28.0
>