diff mbox series

[09/15] net/vmxnet3: switch MSS hint to dynamic mbuf field

Message ID 20201026052105.1561859-10-thomas@monjalon.net (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series remove mbuf userdata |

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Thomas Monjalon Oct. 26, 2020, 5:20 a.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 |  2 ++
 drivers/net/vmxnet3/vmxnet3_rxtx.c   | 10 ++++++----
 3 files changed, 23 insertions(+), 4 deletions(-)

Comments

Andrew Rybchenko Oct. 26, 2020, 3:14 p.m. UTC | #1 
On 10/26/20 8:20 AM, 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>
> ---
>  drivers/net/vmxnet3/vmxnet3_ethdev.c | 15 +++++++++++++++
>  drivers/net/vmxnet3/vmxnet3_ethdev.h |  2 ++
>  drivers/net/vmxnet3/vmxnet3_rxtx.c   | 10 ++++++----
>  3 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> index 6920ab568c..eeb1152b61 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 = "rte_net_vmxnet3_dynfield_segs",
> +		.size = sizeof(uint8_t),
> +		.align = __alignof__(uint8_t),
> +	};
>  
>  	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..5730e94d50 100644
> --- a/drivers/net/vmxnet3/vmxnet3_ethdev.h
> +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.h
> @@ -193,4 +193,6 @@ 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;
> +
>  #endif /* _VMXNET3_ETHDEV_H_ */
> diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c b/drivers/net/vmxnet3/vmxnet3_rxtx.c
> index e10f9ee870..6655622f94 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 = *RTE_MBUF_DYNFIELD(rxm, vmxnet3_segs_dynfield_offset, uint8_t *);
> +	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,8 @@ vmxnet3_rx_offload(struct vmxnet3_hw *hw, const Vmxnet3_RxCompDesc *rcd,
>  					(const Vmxnet3_RxCompDescExt *)rcd;
>  
>  			rxm->tso_segsz = rcde->mss;
> -			rxm->udata64 = rcde->segCnt;
> +			*RTE_MBUF_DYNFIELD(rxm, vmxnet3_segs_dynfield_offset,
> +					uint8_t *) = rcde->segCnt;
>  			ol_flags |= PKT_RX_LRO;
>  		}
>  	} else { /* Offloads set in eop */
> 

I think it should be a rule of thumb to introduce helper
macro to access a dynamic field (as you do in few of
previous patches).

It should be just nearby declaration of the the offset
variable.
Andrew Rybchenko Oct. 26, 2020, 3:21 p.m. UTC | #2 
On 10/26/20 6:14 PM, Andrew Rybchenko wrote:
> On 10/26/20 8:20 AM, 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>
>> ---
>>  drivers/net/vmxnet3/vmxnet3_ethdev.c | 15 +++++++++++++++
>>  drivers/net/vmxnet3/vmxnet3_ethdev.h |  2 ++
>>  drivers/net/vmxnet3/vmxnet3_rxtx.c   | 10 ++++++----
>>  3 files changed, 23 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c b/drivers/net/vmxnet3/vmxnet3_ethdev.c
>> index 6920ab568c..eeb1152b61 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 = "rte_net_vmxnet3_dynfield_segs",
>> +		.size = sizeof(uint8_t),
>> +		.align = __alignof__(uint8_t),
>> +	};
>>  
>>  	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..5730e94d50 100644
>> --- a/drivers/net/vmxnet3/vmxnet3_ethdev.h
>> +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.h
>> @@ -193,4 +193,6 @@ 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;
>> +
>>  #endif /* _VMXNET3_ETHDEV_H_ */
>> diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c b/drivers/net/vmxnet3/vmxnet3_rxtx.c
>> index e10f9ee870..6655622f94 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 = *RTE_MBUF_DYNFIELD(rxm, vmxnet3_segs_dynfield_offset, uint8_t *);
>> +	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,8 @@ vmxnet3_rx_offload(struct vmxnet3_hw *hw, const Vmxnet3_RxCompDesc *rcd,
>>  					(const Vmxnet3_RxCompDescExt *)rcd;
>>  
>>  			rxm->tso_segsz = rcde->mss;
>> -			rxm->udata64 = rcde->segCnt;
>> +			*RTE_MBUF_DYNFIELD(rxm, vmxnet3_segs_dynfield_offset,
>> +					uint8_t *) = rcde->segCnt;
>>  			ol_flags |= PKT_RX_LRO;
>>  		}
>>  	} else { /* Offloads set in eop */
>>
> 
> I think it should be a rule of thumb to introduce helper
> macro to access a dynamic field (as you do in few of
> previous patches).
> 
> It should be just nearby declaration of the the offset
> variable.
> 

May be inline function is even better since, IMHO, if you
both ways are possible, inline function is the right choice.
In this particular case inline function will not add value
from type safety point of view, but it is still better as
an example to follow. In some case inline function could be
used as a place to put build assertion to check size.
Thomas Monjalon Oct. 26, 2020, 4:50 p.m. UTC | #3 
26/10/2020 16:21, Andrew Rybchenko:
> On 10/26/20 6:14 PM, Andrew Rybchenko wrote:
> > On 10/26/20 8:20 AM, Thomas Monjalon wrote:
> >> -			rxm->udata64 = rcde->segCnt;
> >> +			*RTE_MBUF_DYNFIELD(rxm, vmxnet3_segs_dynfield_offset,
> >> +					uint8_t *) = rcde->segCnt;
> > 
> > I think it should be a rule of thumb to introduce helper
> > macro to access a dynamic field (as you do in few of
> > previous patches).
> > 
> > It should be just nearby declaration of the the offset
> > variable.
> 
> May be inline function is even better since, IMHO, if you
> both ways are possible, inline function is the right choice.
> In this particular case inline function will not add value
> from type safety point of view, but it is still better as
> an example to follow. In some case inline function could be
> used as a place to put build assertion to check size.

OK I will review the patches to provide a static inline getter function
for each dynamic field.
Thomas Monjalon Oct. 26, 2020, 6:13 p.m. UTC | #4 
26/10/2020 17:50, Thomas Monjalon:
> 26/10/2020 16:21, Andrew Rybchenko:
> > On 10/26/20 6:14 PM, Andrew Rybchenko wrote:
> > > On 10/26/20 8:20 AM, Thomas Monjalon wrote:
> > >> -			rxm->udata64 = rcde->segCnt;
> > >> +			*RTE_MBUF_DYNFIELD(rxm, vmxnet3_segs_dynfield_offset,
> > >> +					uint8_t *) = rcde->segCnt;
> > > 
> > > I think it should be a rule of thumb to introduce helper
> > > macro to access a dynamic field (as you do in few of
> > > previous patches).
> > > 
> > > It should be just nearby declaration of the the offset
> > > variable.
> > 
> > May be inline function is even better since, IMHO, if you
> > both ways are possible, inline function is the right choice.
> > In this particular case inline function will not add value
> > from type safety point of view, but it is still better as
> > an example to follow. In some case inline function could be
> > used as a place to put build assertion to check size.
> 
> OK I will review the patches to provide a static inline getter function
> for each dynamic field.

After more thoughts, I think it is good to have static inline
returning a pointer to the field in general case:

static inline my_type *
rte_foo_dynfield_bar(struct rte_mbuf * mbuf)
{
	return RTE_MBUF_DYNFIELD(mbuf, my_dynfield_offset, my_type *);
}

But when the scope is limited, it is easier to read a macro
which contains dereferencing of the field pointer with the right type:

#define MY_DYNFIELD(mbuf) \
	(*RTE_MBUF_DYNFIELD(mbuf, my_dynfield_offset, my_type *))

If type cast is never needed, this form allows simple assignment:
	MY_DYNFIELD(mbuf) = field_value;

I would tend to use this latter form for tests or small apps.
diff mbox series

Patch

diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c b/drivers/net/vmxnet3/vmxnet3_ethdev.c
index 6920ab568c..eeb1152b61 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 = "rte_net_vmxnet3_dynfield_segs",
+		.size = sizeof(uint8_t),
+		.align = __alignof__(uint8_t),
+	};
 
 	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..5730e94d50 100644
--- a/drivers/net/vmxnet3/vmxnet3_ethdev.h
+++ b/drivers/net/vmxnet3/vmxnet3_ethdev.h
@@ -193,4 +193,6 @@  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;
+
 #endif /* _VMXNET3_ETHDEV_H_ */
diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c b/drivers/net/vmxnet3/vmxnet3_rxtx.c
index e10f9ee870..6655622f94 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 = *RTE_MBUF_DYNFIELD(rxm, vmxnet3_segs_dynfield_offset, uint8_t *);
+	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,8 @@  vmxnet3_rx_offload(struct vmxnet3_hw *hw, const Vmxnet3_RxCompDesc *rcd,
 					(const Vmxnet3_RxCompDescExt *)rcd;
 
 			rxm->tso_segsz = rcde->mss;
-			rxm->udata64 = rcde->segCnt;
+			*RTE_MBUF_DYNFIELD(rxm, vmxnet3_segs_dynfield_offset,
+					uint8_t *) = rcde->segCnt;
 			ol_flags |= PKT_RX_LRO;
 		}
 	} else { /* Offloads set in eop */