Re: [dpdk-dev] [RFC] ethdev: add sanity packet checks

[Ori Kam <orika@nvidia.com>]

Hi

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> 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:
> > From: Thomas Monjalon
> > > 28/02/2021 20:48, Ori Kam:
> > > > 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
> >
> > > > 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.

> 
> > > > + * RTE_FLOW_ITEM_TYPE_SANITY_CHECKS
> > > > + *
> > > > + * Enable matching on packet validity based on HW checks for the L3 and
> L4
> > > > + * 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.

> > > > +			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.
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.

> 
> > > > +			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,