> -----Original Message-----
> From: Wang, Haiyue <haiyue.w...@intel.com>
> Sent: Tuesday, September 22, 2020 2:09 PM
> To: Guo, Jia <jia....@intel.com>; dev@dpdk.org
> Cc: Zhang, Qi Z <qi.z.zh...@intel.com>; Jiang, JunyuX
> <junyux.ji...@intel.com>; Rong, Leyi <leyi.r...@intel.com>; Yang, Qiming
> <qiming.y...@intel.com>; Sun, GuinanX <guinanx....@intel.com>; Guo,
> Junfeng <junfeng....@intel.com>
> Subject: RE: [PATCH v3] net/ice: refactor the Rx FlexiMD handling
>
> > -----Original Message-----
> > From: Guo, Jia <jia....@intel.com>
> > Sent: Tuesday, September 22, 2020 13:51
> > To: Wang, Haiyue <haiyue.w...@intel.com>; dev@dpdk.org
> > Cc: Zhang, Qi Z <qi.z.zh...@intel.com>; Jiang, JunyuX
> > <junyux.ji...@intel.com>; Rong, Leyi <leyi.r...@intel.com>; Yang,
> > Qiming <qiming.y...@intel.com>; Sun, GuinanX <guinanx....@intel.com>;
> > Guo, Junfeng <junfeng....@intel.com>
> > Subject: RE: [PATCH v3] net/ice: refactor the Rx FlexiMD handling
> >
> >
> > > -----Original Message-----
> > > From: Wang, Haiyue <haiyue.w...@intel.com>
> > > Sent: Tuesday, September 22, 2020 1:42 PM
> > > To: Guo, Jia <jia....@intel.com>; dev@dpdk.org
> > > Cc: Zhang, Qi Z <qi.z.zh...@intel.com>; Jiang, JunyuX
> > > <junyux.ji...@intel.com>; Rong, Leyi <leyi.r...@intel.com>; Yang,
> > > Qiming <qiming.y...@intel.com>; Sun, GuinanX
> > > <guinanx....@intel.com>; Guo, Junfeng <junfeng....@intel.com>
> > > Subject: RE: [PATCH v3] net/ice: refactor the Rx FlexiMD handling
> > >
> > > Hi Jeff,
> > >
> > > > -----Original Message-----
> > > > From: Guo, Jia <jia....@intel.com>
> > > > Sent: Tuesday, September 22, 2020 13:35
> > > > To: Wang, Haiyue <haiyue.w...@intel.com>; dev@dpdk.org
> > > > Cc: Zhang, Qi Z <qi.z.zh...@intel.com>; Jiang, JunyuX
> > > > <junyux.ji...@intel.com>; Rong, Leyi <leyi.r...@intel.com>; Yang,
> > > > Qiming <qiming.y...@intel.com>; Sun, GuinanX
> > > > <guinanx....@intel.com>; Guo, Junfeng <junfeng....@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
>
Sorry, let me show clear as below
#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
if (desc->flow_id != 0xFFFFFFFF) {
mb->ol_flags |= PKT_RX_FDIR | PKT_RX_FDIR_ID;
mb->hash.fdir.hi = rte_le_to_cpu_32(desc->flow_id);
}
>
> 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. ;-)
>
>
That is a good idea.
> > > > > +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.
>
Ok, just clarify me now.
> > > > > +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: /* .... */ ?
>
Yes.
> > > >
> > > > > ice_rx_release_mbufs_t rx_rel_mbufs; };
> > > > >
> > > > > --
> > > > > 2.28.0
> > > >
> > >
> >
>
