[dpdk-dev] [PATCH 1/3] ethdev: support API to set max LRO packet size

[Dekel Peled]

Thanks, PSB.

> -----Original Message-----
> From: Andrew Rybchenko <arybchenko at solarflare.com>
> Sent: Tuesday, November 5, 2019 4:27 PM
> To: Dekel Peled <dekelp at mellanox.com>; john.mcnamara at intel.com;
> marko.kovacevic at intel.com; nhorman at tuxdriver.com;
> ajit.khaparde at broadcom.com; somnath.kotur at broadcom.com;
> anatoly.burakov at intel.com; xuanziyang2 at huawei.com;
> cloud.wangxiaoyun at huawei.com; zhouguoyang at huawei.com;
> wenzhuo.lu at intel.com; konstantin.ananyev at intel.com; Matan Azrad
> <matan at mellanox.com>; Shahaf Shuler <shahafs at mellanox.com>; Slava
> Ovsiienko <viacheslavo at mellanox.com>; rmody at marvell.com;
> shshaikh at marvell.com; maxime.coquelin at redhat.com;
> tiwei.bie at intel.com; zhihong.wang at intel.com; yongwang at vmware.com;
> Thomas Monjalon <thomas at monjalon.net>; ferruh.yigit at intel.com;
> jingjing.wu at intel.com; bernard.iremonger at intel.com
> Cc: dev at dpdk.org
> Subject: Re: [PATCH 1/3] ethdev: support API to set max LRO packet size
> 
> On 11/5/19 5:18 PM, Dekel Peled wrote:
> > Thanks, PSB.
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <arybchenko at solarflare.com>
> >> Sent: Tuesday, November 5, 2019 2:40 PM
> >> To: Dekel Peled <dekelp at mellanox.com>; john.mcnamara at intel.com;
> >> marko.kovacevic at intel.com; nhorman at tuxdriver.com;
> >> ajit.khaparde at broadcom.com; somnath.kotur at broadcom.com;
> >> anatoly.burakov at intel.com; xuanziyang2 at huawei.com;
> >> cloud.wangxiaoyun at huawei.com; zhouguoyang at huawei.com;
> >> wenzhuo.lu at intel.com; konstantin.ananyev at intel.com; Matan Azrad
> >> <matan at mellanox.com>; Shahaf Shuler <shahafs at mellanox.com>; Slava
> >> Ovsiienko <viacheslavo at mellanox.com>; rmody at marvell.com;
> >> shshaikh at marvell.com; maxime.coquelin at redhat.com;
> >> tiwei.bie at intel.com; zhihong.wang at intel.com;
> yongwang at vmware.com;
> >> Thomas Monjalon <thomas at monjalon.net>; ferruh.yigit at intel.com;
> >> jingjing.wu at intel.com; bernard.iremonger at intel.com
> >> Cc: dev at dpdk.org
> >> Subject: Re: [PATCH 1/3] ethdev: support API to set max LRO packet
> >> size
> >>
> >> On 11/5/19 11:40 AM, Dekel Peled wrote:
> >>> This patch implements [1], to support API for configuration and
> >>> validation of max size for LRO aggregated packet.
> >>> API change notice [2] is removed, and release notes for 19.11 are
> >>> updated accordingly.
> >>>
> >>> Various PMDs using LRO offload are updated, the new data members
> are
> >>> initialized to ensure they don't fail validation.
> >>>
> >>> [1]
> >>>
> >>
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatc
> >> h
> >>>
> >>
> es.dpdk.org%2Fpatch%2F58217%2F&data=02%7C01%7Cdekelp%40mell
> >> anox.co
> >>>
> >>
> m%7C751aa0cb18b94b8a447c08d761ed4051%7Ca652971c7d2e4d9ba6a4d149
> >> 256f461
> >>>
> >>
> b%7C0%7C1%7C637085543948425032&sdata=C2laHnaMCQZbDUneQD0
> >> 2Kpi5iAcr%
> >>> 2FYDAS%2BMuO8IcV9s%3D&reserved=0
> >>> [2]
> >>>
> >>
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatc
> >> h
> >>>
> >>
> es.dpdk.org%2Fpatch%2F57492%2F&data=02%7C01%7Cdekelp%40mell
> >> anox.co
> >>>
> >>
> m%7C751aa0cb18b94b8a447c08d761ed4051%7Ca652971c7d2e4d9ba6a4d149
> >> 256f461
> >>>
> >>
> b%7C0%7C1%7C637085543948435028&sdata=XnexdrRYNmFyLqT9IL6ZKa
> >> CLF2JKr
> >>> oKPDVML7gXKceE%3D&reserved=0
> >>>
> >>> Signed-off-by: Dekel Peled <dekelp at mellanox.com>
> >>
> >> Few comments below, otherwise
> >>
> >> Reviewed-by: Andrew Rybchenko <arybchenko at solarflare.com>
> 
> [snip]
> 
> >>> diff --git a/lib/librte_ethdev/rte_ethdev.c
> >>> b/lib/librte_ethdev/rte_ethdev.c index 85ab5f0..2f52090 100644
> >>> --- a/lib/librte_ethdev/rte_ethdev.c
> >>> +++ b/lib/librte_ethdev/rte_ethdev.c
> >>> @@ -1156,6 +1156,26 @@ struct rte_eth_dev *
> >>>  	return name;
> >>>  }
> >>>
> >>> +static inline int
> >>> +rte_eth_check_lro_pkt_size(uint16_t port_id, uint32_t config_size,
> >>> +			   uint32_t dev_info_size)
> >>
> >> As I understand Thomas prefers static functions without rte_eth_ prefix.
> >> I think it is reasonable.
> >
> > Will remove prefix.
> >
> >>
> >>> +{
> >>> +	int ret = 0;
> >>> +
> >>> +	if (config_size > dev_info_size) {
> >>> +		RTE_ETHDEV_LOG(ERR, "Ethdev port_id=%d
> >> max_lro_pkt_size %u > "
> >>> +			       "max allowed value %u\n",
> >>> +			       port_id, config_size, dev_info_size);
> >>> +		ret = -EINVAL;
> >>> +	} else if (config_size < RTE_ETHER_MIN_LEN) {
> >>
> >> Shouldn't config_size == 0 fallback to maximum?
> >> (I don't know and I simply would like to get comments on it)
> >>
> >
> > This check is for value smaller than minimum, not just 0.
> 
> Yes, I know, but the question still remains.

Application may set value 0 explicitly, don't think it should be modified.

> 
> >>> +		RTE_ETHDEV_LOG(ERR, "Ethdev port_id=%d
> >> max_lro_pkt_size %u < "
> >>> +			       "min allowed value %u\n", port_id, config_size,
> >>> +			       (unsigned int)RTE_ETHER_MIN_LEN);
> >>> +		ret = -EINVAL;
> >>> +	}
> >>> +	return ret;
> >>> +}
> >>> +
> >>>  int
> >>>  rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t
> >> nb_tx_q,
> >>>  		      const struct rte_eth_conf *dev_conf) @@ -1286,6
> 
> [snip]