DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Zhang, Helin" <helin.zhang@intel.com>
To: Olivier MATZ <olivier.matz@6wind.com>,
	"Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
	"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] i40e: Use one bit flag for all hardware detected RX packet errors
Date: Fri, 28 Nov 2014 08:07:41 +0000	[thread overview]
Message-ID: <F35DEAC7BCE34641BA9FAC6BCA4A12E70A7CC692@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <5475DFB9.7060609@6wind.com>

Hi Olivier, Konstantin

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Wednesday, November 26, 2014 10:12 PM
> To: Ananyev, Konstantin; Zhang, Helin; dev@dpdk.org
> Cc: Cao, Waterman; Cao, Min
> Subject: Re: [PATCH] i40e: Use one bit flag for all hardware detected RX packet
> errors
> 
> Hi Konstantin,
> 
> On 11/26/2014 02:38 PM, Ananyev, Konstantin wrote:
> >>> Probably I didn't explain myself clear enough, sorry.
> >>> I didn't suggest to get rid of setting bits that indicate L3/L4 checksum errors:
> >>> PKT_RX_IP_CKSUM_BAD, PKT_RX_L4_CKSUM_BAD,
> PKT_RX_EIP_CKSUM_BAD.
> >>> I think these flags should be set as before.
> >>>
> >>> I was talking only about collapsing only these 4 RX error flags into one:
> >>>
> >>> #define PKT_RX_OVERSIZE      (0ULL << 0)  /**< Num of desc of an RX
> pkt oversize. */
> >>> #define PKT_RX_HBUF_OVERFLOW (0ULL << 0)  /**< Header buffer
> overflow. */
> >>> #define PKT_RX_RECIP_ERR     (0ULL << 0)  /**< Hardware processing
> error. */
> >>> #define PKT_RX_MAC_ERR       (0ULL << 0)  /**< MAC error. */
> >>>
> >>>   From my point of view the difference of these 2 groups are:
> >>> First - HW was able to receive whole packet without a problem, but L3/L4
> checksum check failed.
> >>>
> >>> Second - HW was not able to receive whole packet properly by whatever
> reason.
> >>>   From upper layer SW perspective - there it probably makes little
> >>> difference, what caused it, as most likely SW has to throw away erroneous
> packet.
> >>> And for debugging purposes, we can add PMD_LOG(DEBUG, ...) that would
> print what exactly HW error happened.
> >>
> >> I agree with Konstantin that there are 2 different cases:
> >>
> >> a) the packet is properly received by the hardware, but has a bad
> >>      checksum (or another protocol error, for instance an invalid ip len,
> >>      a ip_version == 8 :))
> >>
> >>      in this case, it is useful to the application to have the mbuf with
> >>      the data + an error flag. Then using a tcpdump-like tool could help
> >>      to debug what is the cause of the error and what equipment generates
> >>      a bad packet.
> >>
> >> b) the packet is not properly received by the hardware. In this case
> >>      the data is invalid in the mbuf and not useable by the application.
> >>      I suggest to only have a stats counter in this case, as receiving the
> >>      mbuf is cpu time consuming and the only thing the application can do
> >>      is to drop the packet.
> >
> > So for b) you suggest to drop the packet straight in PMD RX function?
> > Something like:
> > if (unlikely(error_bits & ...)) {
> >          PMD_LOG(DEBUG, ...);
> >           rte_pktmbuf_free(mb);
> > }
> > ?
> 
> Yes
> 
> > That's probably a bit too radical.
> > Yes, mbuf doesn't contain the whole packet, but it may contain at least part of it,
> let say in case of 'packet oversize'.
> > So for debugging purposes the user may still like to examine the mbuf
> contents.
> 
> As soon as there is some exploitable data in the mbuf, I agree it can be transfered
> to the application (ex: bad header, bad len, bad checksum...).
> 
> But if the hardware is not able to provide any exploitable data, it looks a bit
> overkill to give an mbuf with an error flag.
> 
> But grouping the flags as you suggest is already a good clean-up to me, I don't
> want to be more catholic than the Pope ;)

After I have completed another task, I read the datasheet carefully again. For those 5
error bits I introduced for a long time, I'd like to explain one by one as below.

#define PKT_RX_EIP_CKSUM_BAD (0ULL << 0)  /**< External IP header checksum error. */
[Helin] Nobody complains it, so we will keep it there, and just assign a new value to it.

#define PKT_RX_OVERSIZE      (0ULL << 0)  /**< Num of desc of an RX pkt oversize. */
[Helin] I don't think it can be merge with other hardware errors. It indicates the packet
received needs more descriptors than hardware allowed, and the part of packets can
still be stored in the mbufs provided. It is a good hint for users that larger size of mbuf
might be needed. If just put it as hardware error, users will lose this information. So I
prefer to keep it there, and just assign a new value to it.

#define PKT_RX_HBUF_OVERFLOW (0ULL << 0)  /**< Header buffer overflow. */
[Helin] It indicates the header buff size is not enough, but not means hardware cannot
process the packet received. It is a good hint for the users to provide larger size of header
buffers. I also prefer to keep it there, and just assign new value to it.

#define PKT_RX_RECIP_ERR     (0ULL << 0)  /**< Hardware processing error. */
[Helin] In the latest data sheet, it is not opened to external users. So we can just remove
it from here.

#define PKT_RX_MAC_ERR       (0ULL << 0)  /**< MAC error. */
[Helin] This indicates a real hardware error happens.

So my point is to just remove PKT_RX_RECIP_ERR, and we still need other new bit flags.
Any thought from you guys?

Regards,
Helin

> 
> Regards,
> Olivier

  reply	other threads:[~2014-11-28  8:08 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-11-26  6:07 Helin Zhang
2014-11-26 10:49 ` Ananyev, Konstantin
2014-11-26 11:22   ` Olivier MATZ
2014-11-26 13:38     ` Ananyev, Konstantin
2014-11-26 14:12       ` Olivier MATZ
2014-11-28  8:07         ` Zhang, Helin [this message]
2014-11-28  8:47           ` Olivier MATZ
2014-12-01  1:57             ` Zhang, Helin
2014-12-01  9:58               ` Olivier MATZ
2014-12-02  7:25                 ` Zhang, Helin
2014-12-05  1:46 ` [dpdk-dev] [PATCH v2 0/2] fix of enabling all newly added error flags Helin Zhang
2014-12-05  1:46   ` [dpdk-dev] [PATCH v2 1/2] i40e: remove checking rxd flag which is not public Helin Zhang
2014-12-05  1:46   ` [dpdk-dev] [PATCH v2 2/2] mbuf: assign valid bit values for some RX and TX flags Helin Zhang
2014-12-05 10:49     ` Ananyev, Konstantin
2014-12-06  0:42       ` Zhang, Helin
2014-12-06  1:07       ` Zhang, Helin
2014-12-08 10:55         ` Ananyev, Konstantin
2014-12-09  2:29           ` Zhang, Helin
2014-12-06  1:33   ` [dpdk-dev] [PATCH v3] mbuf: fix of enabling all newly added RX error flags Helin Zhang
2014-12-08 10:44     ` Ananyev, Konstantin
2014-12-09  2:23       ` Zhang, Helin
2014-12-08 10:50     ` Thomas Monjalon
2014-12-09  2:14       ` Zhang, Helin
2014-12-09  6:22         ` Thomas Monjalon
2014-12-10  8:55     ` [dpdk-dev] [PATCH v4] " Helin Zhang
2014-12-10  9:35       ` Thomas Monjalon
2014-12-10 13:50         ` Zhang, Helin
2014-12-10 15:26           ` Thomas Monjalon
2014-12-10 22:29             ` Zhang, Helin
2014-12-11 11:16               ` Olivier MATZ
2014-12-12  1:27                 ` Zhang, Helin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=F35DEAC7BCE34641BA9FAC6BCA4A12E70A7CC692@SHSMSX104.ccr.corp.intel.com \
    --to=helin.zhang@intel.com \
    --cc=dev@dpdk.org \
    --cc=konstantin.ananyev@intel.com \
    --cc=olivier.matz@6wind.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).