[dpdk-dev] [PATCH v2 06/13] mbuf: add functions to get the name of an ol_flag

[Ananyev, Konstantin]

> -----Original Message-----
> From: Zhang, Helin
> Sent: Tuesday, November 25, 2014 12:15 PM
> To: Ananyev, Konstantin; 'Olivier MATZ'; 'dev at dpdk.org'
> Cc: 'jigsaw at gmail.com'
> Subject: RE: [dpdk-dev] [PATCH v2 06/13] mbuf: add functions to get the name of an ol_flag
> 
> HI Konstantin
> 
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Tuesday, November 25, 2014 6:38 PM
> > To: 'Olivier MATZ'; 'dev at dpdk.org'
> > Cc: 'jigsaw at gmail.com'; Zhang, Helin
> > Subject: RE: [dpdk-dev] [PATCH v2 06/13] mbuf: add functions to get the name of
> > an ol_flag
> >
> > Hi Helin,
> >
> > > -----Original Message-----
> > > From: Ananyev, Konstantin
> > > Sent: Wednesday, November 19, 2014 11:07 AM
> > > To: Olivier MATZ; dev at dpdk.org
> > > Cc: jigsaw at gmail.com; Zhang, Helin
> > > Subject: RE: [dpdk-dev] [PATCH v2 06/13] mbuf: add functions to get
> > > the name of an ol_flag
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Olivier MATZ [mailto:olivier.matz at 6wind.com]
> > > > Sent: Tuesday, November 18, 2014 9:30 AM
> > > > To: Ananyev, Konstantin; dev at dpdk.org
> > > > Cc: jigsaw at gmail.com; Zhang, Helin
> > > > Subject: Re: [dpdk-dev] [PATCH v2 06/13] mbuf: add functions to get
> > > > the name of an ol_flag
> > > >
> > > > Hi Konstantin,
> > > >
> > > > On 11/17/2014 08:00 PM, Ananyev, Konstantin wrote:
> > > > >> +/*
> > > > >> + * Get the name of a RX offload flag  */ const char
> > > > >> +*rte_get_rx_ol_flag_name(uint64_t mask) {
> > > > >> +	switch (mask) {
> > > > >> +	case PKT_RX_VLAN_PKT: return "PKT_RX_VLAN_PKT";
> > > > >> +	case PKT_RX_RSS_HASH: return "PKT_RX_RSS_HASH";
> > > > >> +	case PKT_RX_FDIR: return "PKT_RX_FDIR";
> > > > >> +	case PKT_RX_L4_CKSUM_BAD: return "PKT_RX_L4_CKSUM_BAD";
> > > > >> +	case PKT_RX_IP_CKSUM_BAD: return "PKT_RX_IP_CKSUM_BAD";
> > > > >> +	/* case PKT_RX_EIP_CKSUM_BAD: return "PKT_RX_EIP_CKSUM_BAD";
> > */
> > > > >> +	/* case PKT_RX_OVERSIZE: return "PKT_RX_OVERSIZE"; */
> > > > >> +	/* case PKT_RX_HBUF_OVERFLOW: return
> > "PKT_RX_HBUF_OVERFLOW"; */
> > > > >> +	/* case PKT_RX_RECIP_ERR: return "PKT_RX_RECIP_ERR"; */
> > > > >> +	/* case PKT_RX_MAC_ERR: return "PKT_RX_MAC_ERR"; */
> > > > >
> > > > > Didn't spot it before, wonder why do you need these 5 commented out
> > lines?
> > > > > In fact, why do we need these flags if they all equal to zero right now?
> > > > > I know these flags were not introduced by that patch, in fact as I
> > > > > can see it was a temporary measure, as old ol_flags were just 16 bits long:
> > > > > http://dpdk.org/ml/archives/dev/2014-June/003308.html
> > > > > So wonder should now these flags either get proper values or be removed?
> > > >
> > > > I would be in favor of removing them, or at least the following ones
> > > > (I don't understand how they can help the application):
> > > >
> > > > - PKT_RX_OVERSIZE: Num of desc of an RX pkt oversize.
> > > > - PKT_RX_HBUF_OVERFLOW: Header buffer overflow.
> > > > - PKT_RX_RECIP_ERR: Hardware processing error.
> > > > - PKT_RX_MAC_ERR: MAC error.
> > >
> > > Tend to agree...
> > > Or probably collapse these 4 flags into one: flag PKT_RX_ERR or something.
> > > Might be still used by someone for debugging purposes.
> > > Helin, what do you think?
> >
> > As there is no answer, I suppose you don't care these flags any more.
> > So we can just remove them, right?
> Sorry, I think I care it a bit. I have a lot of emails to be dealt with, due to the whole week training.
> Yes, it was added there before new mbuf defined. Why zero? Because of lack of bits for them.
> Unfortunately, I forgot to add them with correct values after new mbuf introduced.
> Thank you so much for spotting it out!
> 
> The error flags were added according to the errors defined by FVL datasheet. It could be
> helpful for middle layer software or applications with the specific errors identified. I'd prefer
> to add the correct values for those flags. What do you think?


I am ok to have one flag for that something like PKT_RX_HW_ERR (or something).
Don't really understand why you need all 4 of them - the packet contains invalid data anyway,
so there is not much use of it.
For debugging purposes you can just add a debug log for all of them.
Something like:

if (unlikely(error_bits & ...)) {
     flags |= PKT_RX_MAC_ERR;
     PMD_DRV_LOG(DEBUG, ...);
     return flags;
}

Konstantin

> 
> Thanks and Regards,
> Helin
> 
> >
> > Konstantin
> >
> > >
> > > >
> > > > I would have say that a statistics counter in the driver is more
> > > > appropriate for this case (maybe there is already a counter in the
> > > > hardware).
> > > >
> > > > I have no i40e hardware to test that, so I don't feel very
> > > > comfortable to modify the i40e driver code to add these stats.
> > > >
> > > > Adding Helin in CC list, maybe he has an idea.
> > > >
> > > > Regards,
> > > > Olivier