diff mbox

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

Message ID 1415984609-2484-7-git-send-email-olivier.matz@6wind.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Olivier Matz Nov. 14, 2014, 5:03 p.m. UTC
In test-pmd (rxonly.c), the code is able to dump the list of ol_flags.
The issue is that the list of flags in the application has to be
synchronized with the flags defined in rte_mbuf.h.

This patch introduces 2 new functions rte_get_rx_ol_flag_name()
and rte_get_tx_ol_flag_name() that returns the name of a flag from
its mask. It also fixes rxonly.c to use this new functions and to
display the proper flags.

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 app/test-pmd/rxonly.c      | 36 ++++++++++--------------------------
 lib/librte_mbuf/rte_mbuf.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h | 22 ++++++++++++++++++++++
 3 files changed, 77 insertions(+), 26 deletions(-)

Comments

Bruce Richardson Nov. 17, 2014, 10:39 a.m. UTC | #1
On Fri, Nov 14, 2014 at 06:03:22PM +0100, Olivier Matz wrote:
> In test-pmd (rxonly.c), the code is able to dump the list of ol_flags.
> The issue is that the list of flags in the application has to be
> synchronized with the flags defined in rte_mbuf.h.
> 
> This patch introduces 2 new functions rte_get_rx_ol_flag_name()
> and rte_get_tx_ol_flag_name() that returns the name of a flag from
> its mask. It also fixes rxonly.c to use this new functions and to
> display the proper flags.
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
>  app/test-pmd/rxonly.c      | 36 ++++++++++--------------------------
>  lib/librte_mbuf/rte_mbuf.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  lib/librte_mbuf/rte_mbuf.h | 22 ++++++++++++++++++++++
>  3 files changed, 77 insertions(+), 26 deletions(-)
> 
> diff --git a/app/test-pmd/rxonly.c b/app/test-pmd/rxonly.c
> index 9ad1df6..51a530a 100644
> --- a/app/test-pmd/rxonly.c
> +++ b/app/test-pmd/rxonly.c
> @@ -71,26 +71,6 @@
>  
>  #include "testpmd.h"
>  
> -#define MAX_PKT_RX_FLAGS 13
> -static const char *pkt_rx_flag_names[MAX_PKT_RX_FLAGS] = {
> -	"VLAN_PKT",
> -	"RSS_HASH",
> -	"PKT_RX_FDIR",
> -	"IP_CKSUM",
> -	"IP_CKSUM_BAD",
> -
> -	"IPV4_HDR",
> -	"IPV4_HDR_EXT",
> -	"IPV6_HDR",
> -	"IPV6_HDR_EXT",
> -
> -	"IEEE1588_PTP",
> -	"IEEE1588_TMST",
> -
> -	"TUNNEL_IPV4_HDR",
> -	"TUNNEL_IPV6_HDR",
> -};
> -
>  static inline void
>  print_ether_addr(const char *what, struct ether_addr *eth_addr)
>  {
> @@ -214,12 +194,16 @@ pkt_burst_receive(struct fwd_stream *fs)
>  		printf(" - Receive queue=0x%x", (unsigned) fs->rx_queue);
>  		printf("\n");
>  		if (ol_flags != 0) {
> -			int rxf;
> -
> -			for (rxf = 0; rxf < MAX_PKT_RX_FLAGS; rxf++) {
> -				if (ol_flags & (1 << rxf))
> -					printf("  PKT_RX_%s\n",
> -					       pkt_rx_flag_names[rxf]);
> +			unsigned rxf;
> +			const char *name;
> +
> +			for (rxf = 0; rxf < sizeof(mb->ol_flags) * 8; rxf++) {
> +				if ((ol_flags & (1ULL << rxf)) == 0)
> +					continue;
> +				name = rte_get_rx_ol_flag_name(1ULL << rxf);
> +				if (name == NULL)
> +					continue;
> +				printf("  %s\n", name);
>  			}
>  		}
>  		rte_pktmbuf_free(mb);
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 52e7574..5cd9137 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -196,3 +196,48 @@ rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len)
>  		nb_segs --;
>  	}
>  }
> +
> +/*
> + * 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"; */
> +	case PKT_RX_IPV4_HDR: return "PKT_RX_IPV4_HDR";
> +	case PKT_RX_IPV4_HDR_EXT: return "PKT_RX_IPV4_HDR_EXT";
> +	case PKT_RX_IPV6_HDR: return "PKT_RX_IPV6_HDR";
> +	case PKT_RX_IPV6_HDR_EXT: return "PKT_RX_IPV6_HDR_EXT";
> +	case PKT_RX_IEEE1588_PTP: return "PKT_RX_IEEE1588_PTP";
> +	case PKT_RX_IEEE1588_TMST: return "PKT_RX_IEEE1588_TMST";
> +	case PKT_RX_TUNNEL_IPV4_HDR: return "PKT_RX_TUNNEL_IPV4_HDR";
> +	case PKT_RX_TUNNEL_IPV6_HDR: return "PKT_RX_TUNNEL_IPV6_HDR";
> +	default: return NULL;
> +	}
> +}
> +
> +/*
> + * Get the name of a TX offload flag
> + */
> +const char *rte_get_tx_ol_flag_name(uint64_t mask)
> +{
> +	switch (mask) {
> +	case PKT_TX_VLAN_PKT: return "PKT_TX_VLAN_PKT";
> +	case PKT_TX_IP_CKSUM: return "PKT_TX_IP_CKSUM";
> +	case PKT_TX_TCP_CKSUM: return "PKT_TX_TCP_CKSUM";
> +	case PKT_TX_SCTP_CKSUM: return "PKT_TX_SCTP_CKSUM";
> +	case PKT_TX_UDP_CKSUM: return "PKT_TX_UDP_CKSUM";
> +	case PKT_TX_IEEE1588_TMST: return "PKT_TX_IEEE1588_TMST";
> +	case PKT_TX_VXLAN_CKSUM: return "PKT_TX_VXLAN_CKSUM";
> +	default: return NULL;
> +	}
> +}
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 68fb988..e76617f 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -129,6 +129,28 @@ extern "C" {
>  /* Use final bit of flags to indicate a control mbuf */
>  #define CTRL_MBUF_FLAG       (1ULL << 63) /**< Mbuf contains control data */
>  

I think this patch should perhaps also add to a comment at the top of the list
of flags that any new flags added should also be added to the appropriate
function in rte_mbuf.c. Having the comment in rte_mbuf.h where people would add the flags
should help remind people to keep the flag lists in sync.

/Bruce

> +/**
> + * Get the name of a RX offload flag
> + *
> + * @param mask
> + *   The mask describing the flag.
> + * @return
> + *   The name of this flag, or NULL if it's not a valid RX flag.
> + */
> +const char *rte_get_rx_ol_flag_name(uint64_t mask);
> +
> +/**
> + * Get the name of a TX offload flag
> + *
> + * @param mask
> + *   The mask describing the flag. Usually only one bit must be set.
> + *   Several bits can be given if they belong to the same mask.
> + *   Ex: PKT_TX_L4_MASK.
> + * @return
> + *   The name of this flag, or NULL if it's not a valid TX flag.
> + */
> +const char *rte_get_tx_ol_flag_name(uint64_t mask);
> +
>  /* define a set of marker types that can be used to refer to set points in the
>   * mbuf */
>  typedef void    *MARKER[0];   /**< generic marker for a point in a structure */
> -- 
> 2.1.0
>
Olivier Matz Nov. 17, 2014, 12:51 p.m. UTC | #2
Hi Bruce,

On 11/17/2014 11:39 AM, Bruce Richardson wrote:
>> +/*
>> + * Get the name of a TX offload flag
>> + */
>> +const char *rte_get_tx_ol_flag_name(uint64_t mask)
>> +{
>> +	switch (mask) {
>> +	case PKT_TX_VLAN_PKT: return "PKT_TX_VLAN_PKT";
>> +	case PKT_TX_IP_CKSUM: return "PKT_TX_IP_CKSUM";
>> +	case PKT_TX_TCP_CKSUM: return "PKT_TX_TCP_CKSUM";
>> +	case PKT_TX_SCTP_CKSUM: return "PKT_TX_SCTP_CKSUM";
>> +	case PKT_TX_UDP_CKSUM: return "PKT_TX_UDP_CKSUM";
>> +	case PKT_TX_IEEE1588_TMST: return "PKT_TX_IEEE1588_TMST";
>> +	case PKT_TX_VXLAN_CKSUM: return "PKT_TX_VXLAN_CKSUM";
>> +	default: return NULL;
>> +	}
>> +}
>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>> index 68fb988..e76617f 100644
>> --- a/lib/librte_mbuf/rte_mbuf.h
>> +++ b/lib/librte_mbuf/rte_mbuf.h
>> @@ -129,6 +129,28 @@ extern "C" {
>>  /* Use final bit of flags to indicate a control mbuf */
>>  #define CTRL_MBUF_FLAG       (1ULL << 63) /**< Mbuf contains control data */
>>  
> 
> I think this patch should perhaps also add to a comment at the top of the list
> of flags that any new flags added should also be added to the appropriate
> function in rte_mbuf.c. Having the comment in rte_mbuf.h where people would add the flags
> should help remind people to keep the flag lists in sync.

Good idea, I'll add it.

Regards,
Olivier
Ananyev, Konstantin Nov. 17, 2014, 7 p.m. UTC | #3
Hi Oliver,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> Sent: Friday, November 14, 2014 5:03 PM
> To: dev@dpdk.org
> Cc: jigsaw@gmail.com
> Subject: [dpdk-dev] [PATCH v2 06/13] mbuf: add functions to get the name of an ol_flag
> 
> In test-pmd (rxonly.c), the code is able to dump the list of ol_flags.
> The issue is that the list of flags in the application has to be
> synchronized with the flags defined in rte_mbuf.h.
> 
> This patch introduces 2 new functions rte_get_rx_ol_flag_name()
> and rte_get_tx_ol_flag_name() that returns the name of a flag from
> its mask. It also fixes rxonly.c to use this new functions and to
> display the proper flags.
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
>  app/test-pmd/rxonly.c      | 36 ++++++++++--------------------------
>  lib/librte_mbuf/rte_mbuf.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  lib/librte_mbuf/rte_mbuf.h | 22 ++++++++++++++++++++++
>  3 files changed, 77 insertions(+), 26 deletions(-)
> 
> diff --git a/app/test-pmd/rxonly.c b/app/test-pmd/rxonly.c
> index 9ad1df6..51a530a 100644
> --- a/app/test-pmd/rxonly.c
> +++ b/app/test-pmd/rxonly.c
> @@ -71,26 +71,6 @@
> 
>  #include "testpmd.h"
> 
> -#define MAX_PKT_RX_FLAGS 13
> -static const char *pkt_rx_flag_names[MAX_PKT_RX_FLAGS] = {
> -	"VLAN_PKT",
> -	"RSS_HASH",
> -	"PKT_RX_FDIR",
> -	"IP_CKSUM",
> -	"IP_CKSUM_BAD",
> -
> -	"IPV4_HDR",
> -	"IPV4_HDR_EXT",
> -	"IPV6_HDR",
> -	"IPV6_HDR_EXT",
> -
> -	"IEEE1588_PTP",
> -	"IEEE1588_TMST",
> -
> -	"TUNNEL_IPV4_HDR",
> -	"TUNNEL_IPV6_HDR",
> -};
> -
>  static inline void
>  print_ether_addr(const char *what, struct ether_addr *eth_addr)
>  {
> @@ -214,12 +194,16 @@ pkt_burst_receive(struct fwd_stream *fs)
>  		printf(" - Receive queue=0x%x", (unsigned) fs->rx_queue);
>  		printf("\n");
>  		if (ol_flags != 0) {
> -			int rxf;
> -
> -			for (rxf = 0; rxf < MAX_PKT_RX_FLAGS; rxf++) {
> -				if (ol_flags & (1 << rxf))
> -					printf("  PKT_RX_%s\n",
> -					       pkt_rx_flag_names[rxf]);
> +			unsigned rxf;
> +			const char *name;
> +
> +			for (rxf = 0; rxf < sizeof(mb->ol_flags) * 8; rxf++) {
> +				if ((ol_flags & (1ULL << rxf)) == 0)
> +					continue;
> +				name = rte_get_rx_ol_flag_name(1ULL << rxf);
> +				if (name == NULL)
> +					continue;
> +				printf("  %s\n", name);
>  			}
>  		}
>  		rte_pktmbuf_free(mb);
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 52e7574..5cd9137 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -196,3 +196,48 @@ rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len)
>  		nb_segs --;
>  	}
>  }
> +
> +/*
> + * 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?

Konstantin

> +	case PKT_RX_IPV4_HDR: return "PKT_RX_IPV4_HDR";
> +	case PKT_RX_IPV4_HDR_EXT: return "PKT_RX_IPV4_HDR_EXT";
> +	case PKT_RX_IPV6_HDR: return "PKT_RX_IPV6_HDR";
> +	case PKT_RX_IPV6_HDR_EXT: return "PKT_RX_IPV6_HDR_EXT";
> +	case PKT_RX_IEEE1588_PTP: return "PKT_RX_IEEE1588_PTP";
> +	case PKT_RX_IEEE1588_TMST: return "PKT_RX_IEEE1588_TMST";
> +	case PKT_RX_TUNNEL_IPV4_HDR: return "PKT_RX_TUNNEL_IPV4_HDR";
> +	case PKT_RX_TUNNEL_IPV6_HDR: return "PKT_RX_TUNNEL_IPV6_HDR";
> +	default: return NULL;
> +	}
> +}
> +
> +/*
> + * Get the name of a TX offload flag
> + */
> +const char *rte_get_tx_ol_flag_name(uint64_t mask)
> +{
> +	switch (mask) {
> +	case PKT_TX_VLAN_PKT: return "PKT_TX_VLAN_PKT";
> +	case PKT_TX_IP_CKSUM: return "PKT_TX_IP_CKSUM";
> +	case PKT_TX_TCP_CKSUM: return "PKT_TX_TCP_CKSUM";
> +	case PKT_TX_SCTP_CKSUM: return "PKT_TX_SCTP_CKSUM";
> +	case PKT_TX_UDP_CKSUM: return "PKT_TX_UDP_CKSUM";
> +	case PKT_TX_IEEE1588_TMST: return "PKT_TX_IEEE1588_TMST";
> +	case PKT_TX_VXLAN_CKSUM: return "PKT_TX_VXLAN_CKSUM";
> +	default: return NULL;
> +	}
> +}
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 68fb988..e76617f 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -129,6 +129,28 @@ extern "C" {
>  /* Use final bit of flags to indicate a control mbuf */
>  #define CTRL_MBUF_FLAG       (1ULL << 63) /**< Mbuf contains control data */
> 
> +/**
> + * Get the name of a RX offload flag
> + *
> + * @param mask
> + *   The mask describing the flag.
> + * @return
> + *   The name of this flag, or NULL if it's not a valid RX flag.
> + */
> +const char *rte_get_rx_ol_flag_name(uint64_t mask);
> +
> +/**
> + * Get the name of a TX offload flag
> + *
> + * @param mask
> + *   The mask describing the flag. Usually only one bit must be set.
> + *   Several bits can be given if they belong to the same mask.
> + *   Ex: PKT_TX_L4_MASK.
> + * @return
> + *   The name of this flag, or NULL if it's not a valid TX flag.
> + */
> +const char *rte_get_tx_ol_flag_name(uint64_t mask);
> +
>  /* define a set of marker types that can be used to refer to set points in the
>   * mbuf */
>  typedef void    *MARKER[0];   /**< generic marker for a point in a structure */
> --
> 2.1.0
Olivier Matz Nov. 18, 2014, 9:29 a.m. UTC | #4
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.

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
Ananyev, Konstantin Nov. 19, 2014, 11:06 a.m. UTC | #5
> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, November 18, 2014 9:30 AM
> To: Ananyev, Konstantin; dev@dpdk.org
> Cc: jigsaw@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?

> 
> 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
Ananyev, Konstantin Nov. 25, 2014, 10:37 a.m. UTC | #6
Hi Helin,

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Wednesday, November 19, 2014 11:07 AM
> To: Olivier MATZ; dev@dpdk.org
> Cc: jigsaw@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@6wind.com]
> > Sent: Tuesday, November 18, 2014 9:30 AM
> > To: Ananyev, Konstantin; dev@dpdk.org
> > Cc: jigsaw@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?

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
Helin Zhang Nov. 25, 2014, 12:15 p.m. UTC | #7
HI Konstantin

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Tuesday, November 25, 2014 6:38 PM
> To: 'Olivier MATZ'; 'dev@dpdk.org'
> Cc: 'jigsaw@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@dpdk.org
> > Cc: jigsaw@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@6wind.com]
> > > Sent: Tuesday, November 18, 2014 9:30 AM
> > > To: Ananyev, Konstantin; dev@dpdk.org
> > > Cc: jigsaw@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?

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
Olivier Matz Nov. 25, 2014, 12:37 p.m. UTC | #8
Hi Helin,

On 11/25/2014 01:15 PM, Zhang, Helin wrote:
>>>> 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?

Could you elaborate about why it could be useful for an application
to have this flag in the mbuf? When these flags are set, is the data
still present in the mbuf? How can the application use this data if
the hardware says "there is an error in the packet"?

I think a stats counter would do the job here.

Regards,
Olivier
Helin Zhang Nov. 25, 2014, 1:31 p.m. UTC | #9
Hi Olivier

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, November 25, 2014 8:38 PM
> To: Zhang, Helin; Ananyev, Konstantin; 'dev@dpdk.org'
> Cc: 'jigsaw@gmail.com'
> Subject: Re: [dpdk-dev] [PATCH v2 06/13] mbuf: add functions to get the name of
> an ol_flag
> 
> Hi Helin,
> 
> On 11/25/2014 01:15 PM, Zhang, Helin wrote:
> >>>> 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?
> 
> Could you elaborate about why it could be useful for an application to have this
> flag in the mbuf? When these flags are set, is the data still present in the mbuf?
> How can the application use this data if the hardware says "there is an error in
> the packet"?
That mbuf has already been filled with data, even error happens. The error flags can
be used to indicate if the data is valid or not.
Though it may not need too many error flags, but error flags with specific root causes
could be helpful for users to know what happens.

> 
> I think a stats counter would do the job here.
It already supports statistics collection in i40e.

> 
> Regards,
> Olivier

Regards,
Helin
Ananyev, Konstantin Nov. 25, 2014, 1:49 p.m. UTC | #10
> -----Original Message-----
> From: Zhang, Helin
> Sent: Tuesday, November 25, 2014 12:15 PM
> To: Ananyev, Konstantin; 'Olivier MATZ'; 'dev@dpdk.org'
> Cc: 'jigsaw@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@dpdk.org'
> > Cc: 'jigsaw@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@dpdk.org
> > > Cc: jigsaw@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@6wind.com]
> > > > Sent: Tuesday, November 18, 2014 9:30 AM
> > > > To: Ananyev, Konstantin; dev@dpdk.org
> > > > Cc: jigsaw@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
Helin Zhang Nov. 26, 2014, 12:58 a.m. UTC | #11
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Tuesday, November 25, 2014 9:49 PM
> To: Zhang, Helin; 'Olivier MATZ'; 'dev@dpdk.org'
> Cc: 'jigsaw@gmail.com'
> Subject: RE: [dpdk-dev] [PATCH v2 06/13] mbuf: add functions to get the name of
> an ol_flag
> 
> 
> 
> > -----Original Message-----
> > From: Zhang, Helin
> > Sent: Tuesday, November 25, 2014 12:15 PM
> > To: Ananyev, Konstantin; 'Olivier MATZ'; 'dev@dpdk.org'
> > Cc: 'jigsaw@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@dpdk.org'
> > > Cc: 'jigsaw@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@dpdk.org
> > > > Cc: jigsaw@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@6wind.com]
> > > > > Sent: Tuesday, November 18, 2014 9:30 AM
> > > > > To: Ananyev, Konstantin; dev@dpdk.org
> > > > > Cc: jigsaw@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.
Yes, I agree with you that one bit might be enough. It seems that we have more
than one bits for errors previously.

Regards,
Helin

> 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
diff mbox

Patch

diff --git a/app/test-pmd/rxonly.c b/app/test-pmd/rxonly.c
index 9ad1df6..51a530a 100644
--- a/app/test-pmd/rxonly.c
+++ b/app/test-pmd/rxonly.c
@@ -71,26 +71,6 @@ 
 
 #include "testpmd.h"
 
-#define MAX_PKT_RX_FLAGS 13
-static const char *pkt_rx_flag_names[MAX_PKT_RX_FLAGS] = {
-	"VLAN_PKT",
-	"RSS_HASH",
-	"PKT_RX_FDIR",
-	"IP_CKSUM",
-	"IP_CKSUM_BAD",
-
-	"IPV4_HDR",
-	"IPV4_HDR_EXT",
-	"IPV6_HDR",
-	"IPV6_HDR_EXT",
-
-	"IEEE1588_PTP",
-	"IEEE1588_TMST",
-
-	"TUNNEL_IPV4_HDR",
-	"TUNNEL_IPV6_HDR",
-};
-
 static inline void
 print_ether_addr(const char *what, struct ether_addr *eth_addr)
 {
@@ -214,12 +194,16 @@  pkt_burst_receive(struct fwd_stream *fs)
 		printf(" - Receive queue=0x%x", (unsigned) fs->rx_queue);
 		printf("\n");
 		if (ol_flags != 0) {
-			int rxf;
-
-			for (rxf = 0; rxf < MAX_PKT_RX_FLAGS; rxf++) {
-				if (ol_flags & (1 << rxf))
-					printf("  PKT_RX_%s\n",
-					       pkt_rx_flag_names[rxf]);
+			unsigned rxf;
+			const char *name;
+
+			for (rxf = 0; rxf < sizeof(mb->ol_flags) * 8; rxf++) {
+				if ((ol_flags & (1ULL << rxf)) == 0)
+					continue;
+				name = rte_get_rx_ol_flag_name(1ULL << rxf);
+				if (name == NULL)
+					continue;
+				printf("  %s\n", name);
 			}
 		}
 		rte_pktmbuf_free(mb);
diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 52e7574..5cd9137 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -196,3 +196,48 @@  rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len)
 		nb_segs --;
 	}
 }
+
+/*
+ * 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"; */
+	case PKT_RX_IPV4_HDR: return "PKT_RX_IPV4_HDR";
+	case PKT_RX_IPV4_HDR_EXT: return "PKT_RX_IPV4_HDR_EXT";
+	case PKT_RX_IPV6_HDR: return "PKT_RX_IPV6_HDR";
+	case PKT_RX_IPV6_HDR_EXT: return "PKT_RX_IPV6_HDR_EXT";
+	case PKT_RX_IEEE1588_PTP: return "PKT_RX_IEEE1588_PTP";
+	case PKT_RX_IEEE1588_TMST: return "PKT_RX_IEEE1588_TMST";
+	case PKT_RX_TUNNEL_IPV4_HDR: return "PKT_RX_TUNNEL_IPV4_HDR";
+	case PKT_RX_TUNNEL_IPV6_HDR: return "PKT_RX_TUNNEL_IPV6_HDR";
+	default: return NULL;
+	}
+}
+
+/*
+ * Get the name of a TX offload flag
+ */
+const char *rte_get_tx_ol_flag_name(uint64_t mask)
+{
+	switch (mask) {
+	case PKT_TX_VLAN_PKT: return "PKT_TX_VLAN_PKT";
+	case PKT_TX_IP_CKSUM: return "PKT_TX_IP_CKSUM";
+	case PKT_TX_TCP_CKSUM: return "PKT_TX_TCP_CKSUM";
+	case PKT_TX_SCTP_CKSUM: return "PKT_TX_SCTP_CKSUM";
+	case PKT_TX_UDP_CKSUM: return "PKT_TX_UDP_CKSUM";
+	case PKT_TX_IEEE1588_TMST: return "PKT_TX_IEEE1588_TMST";
+	case PKT_TX_VXLAN_CKSUM: return "PKT_TX_VXLAN_CKSUM";
+	default: return NULL;
+	}
+}
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 68fb988..e76617f 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -129,6 +129,28 @@  extern "C" {
 /* Use final bit of flags to indicate a control mbuf */
 #define CTRL_MBUF_FLAG       (1ULL << 63) /**< Mbuf contains control data */
 
+/**
+ * Get the name of a RX offload flag
+ *
+ * @param mask
+ *   The mask describing the flag.
+ * @return
+ *   The name of this flag, or NULL if it's not a valid RX flag.
+ */
+const char *rte_get_rx_ol_flag_name(uint64_t mask);
+
+/**
+ * Get the name of a TX offload flag
+ *
+ * @param mask
+ *   The mask describing the flag. Usually only one bit must be set.
+ *   Several bits can be given if they belong to the same mask.
+ *   Ex: PKT_TX_L4_MASK.
+ * @return
+ *   The name of this flag, or NULL if it's not a valid TX flag.
+ */
+const char *rte_get_tx_ol_flag_name(uint64_t mask);
+
 /* define a set of marker types that can be used to refer to set points in the
  * mbuf */
 typedef void    *MARKER[0];   /**< generic marker for a point in a structure */