[v2,09/15] net/vmxnet3: switch MSS hint to dynamic mbuf field
diff mbox series

Message ID 20201026222013.2147904-10-thomas@monjalon.net
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • remove mbuf userdata
Related show

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Thomas Monjalon Oct. 26, 2020, 10:20 p.m. UTC
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>
---
 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(-)

Comments

Olivier Matz Oct. 27, 2020, 10:45 a.m. UTC | #1
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
>
Thomas Monjalon Oct. 27, 2020, 4:25 p.m. UTC | #2
27/10/2020 11:45, Olivier Matz:
> 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.

Yes can be a later improvement if the sharing limitations
are properly defined.
We can also share some offsets which are used differently in Rx and Tx.

Patch
diff mbox series

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 */