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

[Guo, Jia]

> -----Original Message-----
> From: Wang, Haiyue <haiyue.wang at intel.com>
> Sent: Tuesday, September 22, 2020 2:24 PM
> To: Guo, Jia <jia.guo at intel.com>; dev at dpdk.org
> Cc: Zhang, Qi Z <qi.z.zhang at intel.com>; Jiang, JunyuX
> <junyux.jiang at intel.com>; Rong, Leyi <leyi.rong at intel.com>; Yang, Qiming
> <qiming.yang at intel.com>; Sun, GuinanX <guinanx.sun at intel.com>; Guo,
> Junfeng <junfeng.guo at intel.com>
> Subject: RE: [PATCH v3] net/ice: refactor the Rx FlexiMD handling
> 
> > -----Original Message-----
> > From: Guo, Jia <jia.guo at intel.com>
> > Sent: Tuesday, September 22, 2020 14:15
> > To: Wang, Haiyue <haiyue.wang at intel.com>; dev at dpdk.org
> > Cc: Zhang, Qi Z <qi.z.zhang at intel.com>; Jiang, JunyuX
> > <junyux.jiang at intel.com>; Rong, Leyi <leyi.rong at intel.com>; Yang,
> > Qiming <qiming.yang at intel.com>; Sun, GuinanX <guinanx.sun at intel.com>;
> > Guo, Junfeng <junfeng.guo at intel.com>
> > Subject: RE: [PATCH v3] net/ice: refactor the Rx FlexiMD handling
> >
> >
> > > -----Original Message-----
> > > From: Wang, Haiyue <haiyue.wang at intel.com>
> > > Sent: Tuesday, September 22, 2020 2:09 PM
> > > To: Guo, Jia <jia.guo at intel.com>; dev at dpdk.org
> > > Cc: Zhang, Qi Z <qi.z.zhang at intel.com>; Jiang, JunyuX
> > > <junyux.jiang at intel.com>; Rong, Leyi <leyi.rong at intel.com>; Yang,
> > > Qiming <qiming.yang at intel.com>; Sun, GuinanX
> > > <guinanx.sun at intel.com>; Guo, Junfeng <junfeng.guo at intel.com>
> > > Subject: RE: [PATCH v3] net/ice: refactor the Rx FlexiMD handling
> > >
> > > > -----Original Message-----
> > > > From: Guo, Jia <jia.guo at intel.com>
> > > > Sent: Tuesday, September 22, 2020 13:51
> > > > To: Wang, Haiyue <haiyue.wang at intel.com>; dev at dpdk.org
> > > > Cc: Zhang, Qi Z <qi.z.zhang at intel.com>; Jiang, JunyuX
> > > > <junyux.jiang at intel.com>; Rong, Leyi <leyi.rong at intel.com>; Yang,
> > > > Qiming <qiming.yang at intel.com>; Sun, GuinanX
> > > > <guinanx.sun at intel.com>; Guo, Junfeng <junfeng.guo at intel.com>
> > > > Subject: RE: [PATCH v3] net/ice: refactor the Rx FlexiMD handling
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Wang, Haiyue <haiyue.wang at intel.com>
> > > > > Sent: Tuesday, September 22, 2020 1:42 PM
> > > > > To: Guo, Jia <jia.guo at intel.com>; dev at dpdk.org
> > > > > Cc: Zhang, Qi Z <qi.z.zhang at intel.com>; Jiang, JunyuX
> > > > > <junyux.jiang at intel.com>; Rong, Leyi <leyi.rong at intel.com>;
> > > > > Yang, Qiming <qiming.yang at intel.com>; Sun, GuinanX
> > > > > <guinanx.sun at intel.com>; Guo, Junfeng <junfeng.guo at intel.com>
> > > > > Subject: RE: [PATCH v3] net/ice: refactor the Rx FlexiMD
> > > > > handling
> > > > >
> > > > > Hi Jeff,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Guo, Jia <jia.guo at intel.com>
> > > > > > Sent: Tuesday, September 22, 2020 13:35
> > > > > > To: Wang, Haiyue <haiyue.wang at intel.com>; dev at dpdk.org
> > > > > > Cc: Zhang, Qi Z <qi.z.zhang at intel.com>; Jiang, JunyuX
> > > > > > <junyux.jiang at intel.com>; Rong, Leyi <leyi.rong at intel.com>;
> > > > > > Yang, Qiming <qiming.yang at intel.com>; Sun, GuinanX
> > > > > > <guinanx.sun at intel.com>; Guo, Junfeng <junfeng.guo at 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; hash.fdir.hi =
> > mb->rte_le_to_cpu_32(desc->flow_id);
> > }
> >
> 
> That's what I said about the order, you can check
> "ice_rxd_to_pkt_fields_by_comms_ovs"
> vs "ice_rxd_to_pkt_fields_by_comms_aux_v1" about "desc->flow_id" and
> "desc->rss_hash"
> handling order: offset first, handle it firstly.
> 

I know your point and you could choose if need more doc to highlight this offset change or not. Anyway, I am not insist object for this order.

> > > > > > > +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.
> 
> OK.
> 
> >
> > > > > >
> > > > > > >  ice_rx_release_mbufs_t rx_rel_mbufs;  };
> > > > > > >
> > > > > > > --
> > > > > > > 2.28.0
> > > > > >
> > > > >
> > > >
> > >
> >
>