On Sun, Mar 7, 2021 at 10:46 AM Ori Kam [off-list ref] wrote:

Hi

quoted

-----Original Message-----
From: Thomas Monjalon <redacted>
Sent: Thursday, March 4, 2021 12:46 PM
Subject: Re: [dpdk-dev] [RFC] ethdev: add sanity packet checks

04/03/2021 11:00, Ori Kam:

quoted

From: Thomas Monjalon

quoted

28/02/2021 20:48, Ori Kam:

quoted

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

s/rte flow/rte_flow/

Sure

quoted

quoted

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.

What is the "full check"?
How much it is HW dependent?

This also relates to your other comments,
Each HW may run different set of checks on the packet,
for example one PMD can just check the tcp flags while
a different PMD will also check the option.

I'm not sure how an application can rely on
such a vague definition.

Even now we are marking a packet in the mbuf with unknown
in case of some error.
Would a better wording be " The HW detected errors in the packet"
in any case if the app will need to know what is the error it is his
responsibility, this item is just verification for fast path.
If you have better suggestion, I will be very happy to hear.

quoted

quoted

quoted

quoted

+ * RTE_FLOW_ITEM_TYPE_SANITY_CHECKS
+ *
+ * Enable matching on packet validity based on HW checks for the L3 and

L4

quoted

quoted

quoted

+ * layers.
+ */
+struct rte_flow_item_sanity_checks {
+       uint32_t level;
+       /**< Packet encapsulation level the item should apply to.
+        * @see rte_flow_action_rss
+        */
+RTE_STD_C11
+       union {
+               struct {

Why there is no L2 check?

Our HW doesn't support it.
If other HW support it, it should be added.

It would be an ABI breakage. Can we add it day one?

Will add reserve, since this is bit field there shouldn't be any
ABI break.

quoted

quoted

quoted

quoted

+                       uint32_t l3_ok:1;
+                       /**< L3 layer is valid after passing all HW checking. */
+                       uint32_t l4_ok:1;
+                       /**< L4 layer is valid after passing all HW checking. */

l3_ok and l4_ok looks vague.
What does it cover exactly?

It depends on the HW in question.
In our case it checks in case of L3
the header len, and the version.
For L4 checking the len.

If we don't know exactly what is checked,
how an application can rely on it?
Is it a best effort check? What is the use case?

From application point of view that packet is invalid.
it is the app responsibility to understand why.

And that it can determine based on the available fields in ol_flags. right?
If HW can indicate that the packet integrity is in question,
a PMD should be able to set the bits in ol_flags. After that
the application should decide what to drop and what to pass.

What is missing is the ability for the application to tell the HW/PMD to
drop any packet which fails packet integrity checks.

I believe generally drop packets when Ethernet CRC check fails.
But l3 and l4 errors are left to the application to deal with.
If an application wants to save some CPU cycles, it could ask the
hardware to drop those packets as well. So one bit to enable/disable
this for all packets should be good.

In case we still want to pursue this per flow, how about
RTE_FLOW_ITEM_TYPE_PACKET_INTEGRITY_CHECKS instead of
RTE_FLOW_ITEM_TYPE_SANITY_CHECKS

You can think about it that in any case there might be
different counters for different errors or different actions.
For example, the app can decide that incorrect ipv4 version
should result in packet drop, while incorrect len
may pass.

Maybe we can list all possible error result, but I'm worried
that we may not cover all of them. On some HW there is just one
bit that marks if the packet is valid or not.

quoted

quoted

quoted

quoted

+                       uint32_t l3_ok_csum:1;
+                       /**< L3 layer checksum is valid. */
+                       uint32_t l4_ok_csum:1;
+                       /**< L4 layer checksum is valid. */

What worries me is that the checksum is separate but other checks
are in a common bucket.
I think we should have one field per precise check
with a way to report what is checked.

Pleas see above comment, Current HW doesn't support
such fine grain detection, so adding bit will force the user
to select all of them, in addition since the HW has some internal
checks it is possible that the reject reason will be un related to the
L3, for example some HW may not support more then two vlans
so in case of 3 vlans may report bad L3.

Best,
Ori,