-----Original Message----- > Date: Wed, 3 Oct 2018 10:34:52 +0300 > From: Andrew Rybchenko <arybche...@solarflare.com> > To: Jerin Jacob <jerin.ja...@caviumnetworks.com>, Wenzhuo Lu > <wenzhuo...@intel.com>, Jingjing Wu <jingjing...@intel.com>, Bernard > Iremonger <bernard.iremon...@intel.com>, John McNamara > <john.mcnam...@intel.com>, Marko Kovacevic <marko.kovace...@intel.com>, > Thomas Monjalon <tho...@monjalon.net>, Ferruh Yigit > <ferruh.yi...@intel.com>, Olivier Matz <olivier.m...@6wind.com> > CC: dev@dpdk.org, shah...@mellanox.com, "Ananyev, Konstantin" > <konstantin.anan...@intel.com> > Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP > checksum definition > User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 > Thunderbird/60.0 > > > On 10/2/18 10:24 PM, Jerin Jacob wrote: > > Introduced DEV_RX_OFFLOAD_OUTER_UDP_CKSUM Rx offload flag and > PKT_RX_EL4_CKSUM_BAD mbuf ol_flags to detect outer UDP checksum > failure. > > - To use hardware Rx outer UDP checksum offload, the user needs to > configure DEV_RX_OFFLOAD_OUTER_UDP_CKSUM offload flags in slowpath. > > - Driver updates the PKT_RX_EL4_CKSUM_BAD mbuf ol_flag on checksum failure > similar to the outer L3 PKT_RX_EIP_CKSUM_BAD flag. > > Signed-off-by: Jerin Jacob > <jerin.ja...@caviumnetworks.com><mailto:jerin.ja...@caviumnetworks.com> > > 1. I'm not sure that it is OK that mbuf and ethdev changes go in one patch. > It seems typically mbuf changes go separately and mbuf changes should > be applied to main dpdk repo.
I don't have strong opinion on this. If there are no other objection, I will split the patch further as mbuf and ethdev as you pointed out. > > 2. I'd like to see thought why single bit is used for outer L2 checksum when > 2 bits (UNKNOWN, BAD, GOOD, NONE) are used for PKT_RX_L4_CKSUM. > May be it is OK, but it would be useful to state explicitly why it is > decided > to go this way. I am following the scheme similar to OUTER IP checksum where we have only one bit filed(PKT_RX_EIP_CKSUM_BAD). I will mention in the git commit. > > 3. PKT_RX_L4_CKSUM_MASK description says nothing if it is inner or outer. > May be it is not directly related to changeset, but I think it would be > really > useful to clarify it. I will update the comment. > > > Plus one nit below. > > > > --- > > v2: > - Removed DEV_RX_OFFLOAD_OUTER_TCP_CKSUM and DEV_RX_OFFLOAD_OUTER_SCTP_CKSUM > as there is no realworld use case for it. > See: http://patches.dpdk.org/patch/44692/ > > This patch series is depended on http://patches.dpdk.org/patch/45840/ > > -- > app/test-pmd/config.c | 9 +++++++++ > doc/guides/nics/features.rst | 3 +++ > lib/librte_ethdev/rte_ethdev.c | 1 + > lib/librte_ethdev/rte_ethdev.h | 1 + > lib/librte_mbuf/rte_mbuf.c | 2 ++ > lib/librte_mbuf/rte_mbuf.h | 3 +++ > 6 files changed, 19 insertions(+) > > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c > index 1adc9b94b..d53c527e5 100644 > --- a/app/test-pmd/config.c > +++ b/app/test-pmd/config.c > @@ -594,6 +594,15 @@ port_offload_cap_display(portid_t port_id) > printf("off\n"); > } > > + if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_OUTER_UDP_CKSUM) { > + printf("RX Outer UDP checksum: "); > + if (ports[port_id].dev_conf.rxmode.offloads & > + DEV_RX_OFFLOAD_OUTER_UDP_CKSUM) > + printf("on\n"); > + else > + printf("off\n"); > + } > + > if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_TCP_LRO) { > printf("Large receive offload: "); > if (ports[port_id].dev_conf.rxmode.offloads & > diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst > index d42489b6d..2c2959e0b 100644 > --- a/doc/guides/nics/features.rst > +++ b/doc/guides/nics/features.rst > @@ -639,6 +639,9 @@ Inner L4 checksum > > Supports inner packet L4 checksum. > > +* **[uses] rte_eth_rxconf,rte_eth_rxmode**: > ``offloads:DEV_RX_OFFLOAD_OUTER_UDP_CKSUM``. > +* **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_EL4_CKSUM_BAD``. > +* **[provides] rte_eth_dev_info**: > ``rx_offload_capa,rx_queue_offload_capa:DEV_RX_OFFLOAD_OUTER_UDP_CKSUM``, > > > One more empty line should be added here to have two empty lines between > features. OK. Thanks for the review. > > Andrew.
