On Sun, Mar 7, 2021 at 10:46 AM Ori Kam wrote: > > Hi > > > -----Original Message----- > > From: Thomas Monjalon > > 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. 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. > > > > > > > > + 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, >