Thanks, PSB. > -----Original Message----- > From: Andrew Rybchenko <arybche...@solarflare.com> > Sent: Tuesday, November 5, 2019 4:27 PM > To: Dekel Peled <dek...@mellanox.com>; john.mcnam...@intel.com; > marko.kovace...@intel.com; nhor...@tuxdriver.com; > ajit.khapa...@broadcom.com; somnath.ko...@broadcom.com; > anatoly.bura...@intel.com; xuanziya...@huawei.com; > cloud.wangxiao...@huawei.com; zhouguoy...@huawei.com; > wenzhuo...@intel.com; konstantin.anan...@intel.com; Matan Azrad > <ma...@mellanox.com>; Shahaf Shuler <shah...@mellanox.com>; Slava > Ovsiienko <viachesl...@mellanox.com>; rm...@marvell.com; > shsha...@marvell.com; maxime.coque...@redhat.com; > tiwei....@intel.com; zhihong.w...@intel.com; yongw...@vmware.com; > Thomas Monjalon <tho...@monjalon.net>; ferruh.yi...@intel.com; > jingjing...@intel.com; bernard.iremon...@intel.com > Cc: dev@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 <arybche...@solarflare.com> > >> Sent: Tuesday, November 5, 2019 2:40 PM > >> To: Dekel Peled <dek...@mellanox.com>; john.mcnam...@intel.com; > >> marko.kovace...@intel.com; nhor...@tuxdriver.com; > >> ajit.khapa...@broadcom.com; somnath.ko...@broadcom.com; > >> anatoly.bura...@intel.com; xuanziya...@huawei.com; > >> cloud.wangxiao...@huawei.com; zhouguoy...@huawei.com; > >> wenzhuo...@intel.com; konstantin.anan...@intel.com; Matan Azrad > >> <ma...@mellanox.com>; Shahaf Shuler <shah...@mellanox.com>; Slava > >> Ovsiienko <viachesl...@mellanox.com>; rm...@marvell.com; > >> shsha...@marvell.com; maxime.coque...@redhat.com; > >> tiwei....@intel.com; zhihong.w...@intel.com; > yongw...@vmware.com; > >> Thomas Monjalon <tho...@monjalon.net>; ferruh.yi...@intel.com; > >> jingjing...@intel.com; bernard.iremon...@intel.com > >> Cc: dev@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 <dek...@mellanox.com> > >> > >> Few comments below, otherwise > >> > >> Reviewed-by: Andrew Rybchenko <arybche...@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]
