Re: [dpdk-dev] [PATCH v3] net/ice: refactor the Rx FlexiMD handling

["Wang, Haiyue" <haiyue.wang@intel.com>]

> -----Original Message-----
> From: Guo, Jia <jia.guo@intel.com>
> Sent: Tuesday, September 22, 2020 13:51
> To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Jiang, JunyuX <junyux.jiang@intel.com>; Rong, Leyi
> <leyi.rong@intel.com>; Yang, Qiming <qiming.yang@intel.com>; Sun, GuinanX <guinanx.sun@intel.com>; Guo,
> Junfeng <junfeng.guo@intel.com>
> Subject: RE: [PATCH v3] net/ice: refactor the Rx FlexiMD handling
> 
> 
> > -----Original Message-----
> > From: Wang, Haiyue <haiyue.wang@intel.com>
> > Sent: Tuesday, September 22, 2020 1:42 PM
> > To: Guo, Jia <jia.guo@intel.com>; dev@dpdk.org
> > Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Jiang, JunyuX
> > <junyux.jiang@intel.com>; Rong, Leyi <leyi.rong@intel.com>; Yang, Qiming
> > <qiming.yang@intel.com>; Sun, GuinanX <guinanx.sun@intel.com>; Guo,
> > Junfeng <junfeng.guo@intel.com>
> > Subject: RE: [PATCH v3] net/ice: refactor the Rx FlexiMD handling
> >
> > Hi Jeff,
> >
> > > -----Original Message-----
> > > From: Guo, Jia <jia.guo@intel.com>
> > > Sent: Tuesday, September 22, 2020 13:35
> > > To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org
> > > Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Jiang, JunyuX
> > > <junyux.jiang@intel.com>; Rong, Leyi <leyi.rong@intel.com>; Yang,
> > > Qiming <qiming.yang@intel.com>; Sun, GuinanX <guinanx.sun@intel.com>;
> > > Guo, Junfeng <junfeng.guo@intel.com>
> > > Subject: RE: [PATCH v3] net/ice: refactor the Rx FlexiMD handling
> > >
> > > Hi, haiyue
> > >

> > > > +   struct rte_mbuf *mb,
> > > > +   volatile union ice_rx_flex_desc *rxdp) { volatile struct
> > > > +ice_32b_rx_flex_desc_comms_ovs *desc = (volatile struct
> > > > +ice_32b_rx_flex_desc_comms_ovs
> > > > *)rxdp; #ifndef
> > > > +RTE_LIBRTE_ICE_16BYTE_RX_DESC
> > > > +uint16_t stat_err;
> > > > +#endif
> > >
> > > This #ifndef could be better combine with below #ifndef.
> > >
> >
> > I changed it to according to the different offsets, like ovs has rss hash in
> > Qword 3, which is after flow Id Qword 1, others are opposite. So that this
> > handling order can reflect the offset difference, although, it MAY looks not
> > so beautiful. What do you think ? :)
> >
> 
> I am not sure I got you point about the order reason, but I think in or out #ifndef should be clear
> show the offset difference, 

You mean below ? If so, 'uint16_t stat_err;' is not so good in the middle of two code blocks.

#ifndef RTE_LIBRTE_ICE_16BYTE_RX_DESC
	uint16_t stat_err;

	stat_err = rte_le_to_cpu_16(desc->status_error0);
	if (likely(stat_err & (1 << ICE_RX_FLEX_DESC_STATUS0_RSS_VALID_S))) {
		mb->ol_flags |= PKT_RX_RSS_HASH;
		mb->hash.rss = rte_le_to_cpu_32(desc->rss_hash);
	}
#endif


I am insist about that but if other also agree. And I still have 2
> comments assume you are default agree and could I expect a new version or not?
> 

Oh, I missed you other two comments. Your reply is embedded into my original many
code lines. You can remove the unnecessary codes, so that people can get your comments
quickly without moving mouse down to check line by line. ;-)


> > > > +if (rxq->xtr_ol_flag) {
> > > > +uint32_t metadata = 0;
> > > > +
> > > > +if (desc->flex_ts.flex.aux0 != 0xFFFF) metadata =
> > > > +rte_le_to_cpu_16(desc-
> > > > >flex_ts.flex.aux0);
> > > > +else if (desc->flex_ts.flex.aux1 != 0xFFFF) metadata =
> > > > +rte_le_to_cpu_16(desc-
> > > > >flex_ts.flex.aux1);
> > >
> > > So you mean the ICE_RX_FLEX_DESC_STATUS1_XTRMD4_VALID_S and
> > > ICE_RX_FLEX_DESC_STATUS1_XTRMD5_VALID_S
> > > could not use to identify the IPv4 hdr offset and IPv6 hdr offset here in
> > rxdid # 25?
> > > And if yes they can and they will not set at the same time, is that separate
> > this v2 from v1 necessary?

XTRMD4/5_VALID_S can't detect the value of IP offset available, non 0xffffffff
is the right check condition.

> > > > +uint64_t xtr_ol_flag; /* Protocol extraction offload flag */
> > > > +ice_rxd_to_pkt_fields_t rxd_to_pkt_fields;
> > >
> > > If create a function pointer here in .h, it is better add some doc.

C comments: /* .... */ ?

> > >
> > > >  ice_rx_release_mbufs_t rx_rel_mbufs;
> > > >  };
> > > >
> > > > --
> > > > 2.28.0
> > >
> >
>