From: Ori Kam <orika@nvidia.com>
To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>,
NBU-Contact-Thomas Monjalon <thomas@monjalon.net>
Cc: Slava Ovsiienko <viacheslavo@nvidia.com>,
"ferruh.yigit@intel.com" <ferruh.yigit@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>,
"ajit.khaparde@broadcom.com" <ajit.khaparde@broadcom.com>,
"olivier.matz@6wind.com" <olivier.matz@6wind.com>,
"jerinj@marvell.com" <jerinj@marvell.com>,
Ori Kam <orika@nvidia.com>
Subject: Re: [dpdk-dev] [RFC] ethdev: add sanity packet checks
Date: Tue, 9 Mar 2021 19:46:08 +0000 [thread overview]
Message-ID: <DM6PR12MB49870DA255279C540357A242D6929@DM6PR12MB4987.namprd12.prod.outlook.com> (raw)
In-Reply-To: <057b06ac-6c52-3f21-9e2f-cfdd66651342@oktetlabs.ru>
Hi Andrew,
Thanks for the reply PDB,
> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Tuesday, March 9, 2021 5:28 PM
> Subject: Re: [dpdk-dev] [RFC] ethdev: add sanity packet checks
>
> On 3/9/21 6:08 PM, Ori Kam wrote:
> > Hi
> >
> >> -----Original Message-----
> >> From: dev <dev-bounces@dpdk.org> On Behalf Of Thomas Monjalon
> >> Sent: Tuesday, March 9, 2021 11:11 AM
> >> Subject: Re: [dpdk-dev] [RFC] ethdev: add sanity packet checks
> >>
> >> 09/03/2021 10:01, Andrew Rybchenko:
> >>> On 2/28/21 10:48 PM, Ori Kam wrote:
> >>>> Currently, DPDK application can offload the checksum check,
> >>>> and report it in the mbuf.
> >>>>
> >>>> However, this approach doesn't work if the traffic
> >>>> is offloaded and should not arrive to the application.
> >>>>
> >>>> This commit introduces rte flow item that enables
> >>>> matching on the checksum of the L3 and L4 layers,
> >>>> in addition to other checks that can determine if
> >>>> the packet is valid.
> >>>> some of those tests can be packet len, data len,
> >>>> unsupported flags, and so on.
> >>>>
> >>>> The full check is HW dependent.
> >>>>
> >>>> Signed-off-by: Ori Kam <orika@nvidia.com>
> >>>
> >>> In general, I strongly dislike the approach. If such checks are required,
> >>> it must be done per item basis. I.e. we should add non-header boolean
> >>> flags to IPv4, TCP, UDP etc items. E.g.
> >>>
> >>> struct rte_flow_item_ipv4 {
> >>> struct rte_ipv4_hdr hdr; /**< IPv4 header definition. */
> >>> bool hdr_checksum_valid;
> >>> };
> >>>
> >>> Also it will allow to filter by packets with invalid checksum as well and
> >>> redirect to dedicated Rx path or drop and/or count etc.
> >>
> >> +1
> >>
> > I'm not sure I understand your comment, also according to the proposed
> > RFC we can redirect to the correct path.
> >
> >> I think the only drawback of this solution is for HW giving a global
> >> check status without knowing which header is valid or invalid.
> >>
> > Like Thomas remark the main drawback with adding the valid to each
> > of the items is that, it forces the application to have detected rule per
> > each type, which will make the SW logic more complex.
> > Also this may have performance impact on the packets and on the
> > number of flows.
> >
>
> If we try to match on something which is a part of the packet header X,
> it must be done in a flow item which corresponds to
> protocol X. Otherwise, we have two items which overlap.
> Also approach suggested in RFC break networking protocol layering and
> features of different layers are mixed in one
> item. What will you when you need to support tunnels? Add
> inner/outer fields? Sorry, it is ugly. If valid controls are
> added in protocol items, no specific efforts are required for
> tunnels.
>
I don't think it breaks protocol layers. This is a very common use case that
application before touching the packet wants to make sure that the packet
is valid and not waste extra hops just to understand at the end that it is a bad packet.
Also even now we report in the mbuf the status of the L3,L4 check sum and status (using
the packet state)
About the tunnel we can solve it by adding level just like in the RSS case.
One more thing to consider is that adding the new bits to existing API
will break the API/ABI.
> Also approach suggested in RFC requires very careful definition
> what happens if a packet does not have corresponding layer but
> matching on valid or invalid checksum is requested.
>
I would say this is app issue, just like modify TTL in IPV4 if there is
no item then it is the app issue,
> IMHO a vendor HW limitations are out-of-scope of the
> generic API definition. May be one day I will regret about
> these words, but I'm still sure that from API point of view
> solution suggested by me above is better.
I fully agree (I may also regret those words) that HW limitations should be
out of scope unless they are in some form of hint which number of HW
can use to increase performance.
As far as I can see the only difference between the your suggestion and the RFC
is that in the RFC there is a new dedicated item for such checks, while in your
suggestion we will add those bits to each of the items. (I assume that
also in your suggestion we will add more than just the check sum).
The main draw backs from my point of view of your suggestion
1. ABI/AFI break.
2. Application is force to insert more rules.
Best,
Ori
prev parent reply other threads:[~2021-03-09 19:46 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-28 19:48 Ori Kam
2021-02-28 20:14 ` Thomas Monjalon
2021-03-04 10:00 ` Ori Kam
2021-03-04 10:46 ` Thomas Monjalon
2021-03-07 18:46 ` Ori Kam
2021-03-08 23:05 ` Ajit Khaparde
2021-03-09 19:21 ` Ori Kam
2021-03-09 9:01 ` Andrew Rybchenko
2021-03-09 9:11 ` Thomas Monjalon
2021-03-09 15:08 ` Ori Kam
2021-03-09 15:27 ` Andrew Rybchenko
2021-03-09 19:46 ` Ori Kam [this message]
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=DM6PR12MB49870DA255279C540357A242D6929@DM6PR12MB4987.namprd12.prod.outlook.com \
--to=orika@nvidia.com \
--cc=ajit.khaparde@broadcom.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=jerinj@marvell.com \
--cc=olivier.matz@6wind.com \
--cc=thomas@monjalon.net \
--cc=viacheslavo@nvidia.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).