Bug 2076506

Summary: [ERFE] Use kfree_skb_reason in OVS kernel datapath
Product: Red Hat Enterprise Linux Fast Datapath Reporter: Adrián Moreno <amorenoz>
Component: openvswitch2.17Assignee: Adrián Moreno <amorenoz>
Status: MODIFIED --- QA Contact: Rick Alongi <ralongi>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: FDP 20.ECC: ctrautma, echaudro, egarver, i.maximets, jhsiao, ralongi
Target Milestone: ---   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Adrián Moreno 2022-04-19 08:58:32 UTC
Calls to kfree_skb() are being replaced with kfree_skb_reason() in several parts of the networking stack to provide more visibility on drops.

We should consider adding some reasons to the datapath's drops.

Comment 1 Eric Garver 2023-06-30 12:51:41 UTC
v1 series posted. v2 will be posted in a couple weeks after net-next opens.

kernel: https://marc.info/?t=168807084300005&r=1&w=2
OVS: https://mail.openvswitch.org/pipermail/ovs-dev/2023-June/406032.html

Comment 2 Adrián Moreno 2023-06-30 14:38:50 UTC
Based on Eric's work, and in order to also provide visibility on other drops I was planning to:
- Add another drop-reason subsystem for OVS datapath drops (as opposed to xlate ones that Eric has added)
- Add a drop reason for obvious flow based drops (e.g: empty action list)
- Add a drop reason for upcall drop (netlink buffer overflow)
- Add a drop reason for meters

Also allow for userspace to add an explicit non-error DROP action.

A problem I see is sampling. We can add IPFIX sampling of packets that end up being dropped with a non-empty action list but still packets end up being dropped. I hope we can address that in the xlate layer and add an explicit drop in that case.
Same (probably even more complex) can happen with clone().

But even without the complex cases, I hope we can add visibility to most of the common drops.
Thoughts?

Comment 3 Ilya Maximets 2023-06-30 15:16:54 UTC
I agree that an explicit drop action and this BZ do overlap, but do not replace
each other.  And it is very valuable to trace all the other types of packet
drops in the kernel module, not only specific flow translation reasons.

I suppose we could have 2 different sets of subsytem reasons for OVS, right?
One for XLATE and one generic?  E.g., SUBSYS_MAC80211 has two sets of reasons.

Should we create a separate BZ for the Eric's current work?  It may also need
two BZs on its own, one for kernel and one for userspace.

Comment 4 Eric Garver 2023-06-30 17:43:34 UTC
(In reply to Ilya Maximets from comment #3)
> I agree that an explicit drop action and this BZ do overlap, but do not
> replace
> each other.  And it is very valuable to trace all the other types of packet
> drops in the kernel module, not only specific flow translation reasons.
> 
> I suppose we could have 2 different sets of subsytem reasons for OVS, right?
> One for XLATE and one generic?  E.g., SUBSYS_MAC80211 has two sets of
> reasons.

Yes. Adrian and I agreed offline to use two separate subsystem IDs.
There are 2^16 IDs available, so lets use them. ;)

> Should we create a separate BZ for the Eric's current work?  It may also need
> two BZs on its own, one for kernel and one for userspace.

Agree. I will create new bugs for xlate_error (my work).

Comment 5 Eric Garver 2023-06-30 17:51:51 UTC
> > Should we create a separate BZ for the Eric's current work?  It may also need
> > two BZs on its own, one for kernel and one for userspace.
> 
> Agree. I will create new bugs for xlate_error (my work).

Created.

kernel Bug 2218963
FDP Bug 2218964