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

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

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.