Re: [net-next v3 1/7] net: openvswitch: add datapath flow drop reason

[Adrian Moreno <amorenoz@redhat.com>]

On 8/8/23 20:02, Ilya Maximets wrote:
> On 8/7/23 18:45, Adrian Moreno wrote:
>> Create a new drop reason subsystem for openvswitch and add the first
>> drop reason to represent flow drops.
>>
>> A flow drop happens when a flow has an empty action-set or there is no
>> action that consumes the packet (output, userspace, recirc, etc).
>>
>> Implementation-wise, most of these skb-consuming actions already call
>> "consume_skb" internally and return directly from within the
>> do_execute_actions() loop so with minimal changes we can assume that
>> any skb that exits the loop normally is a packet drop.
>>
>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>> ---
>>   include/net/dropreason.h   |  6 ++++++
>>   net/openvswitch/actions.c  | 12 ++++++++++--
>>   net/openvswitch/datapath.c | 16 ++++++++++++++++
>>   net/openvswitch/drop.h     | 24 ++++++++++++++++++++++++
>>   4 files changed, 56 insertions(+), 2 deletions(-)
>>   create mode 100644 net/openvswitch/drop.h
> 
> <snip>
> 
>> diff --git a/net/openvswitch/drop.h b/net/openvswitch/drop.h
>> new file mode 100644
>> index 000000000000..cdd10629c6be
>> --- /dev/null
>> +++ b/net/openvswitch/drop.h
>> @@ -0,0 +1,24 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * OpenvSwitch drop reason list.
>> + */
>> +
>> +#ifndef OPENVSWITCH_DROP_H
>> +#define OPENVSWITCH_DROP_H
>> +#include <net/dropreason.h>
>> +
>> +#define OVS_DROP_REASONS(R)			\
>> +	R(OVS_DROP_FLOW)		        \
> 
> Hi, Adrian.  Not a full review, just complaining about names. :)
> 
> The OVS_DROP_FLOW seems a bit confusing and unclear.  A "flow drop"
> is also a strange term to use.  Maybe we can somehow express in the
> name that this drop reason is used when there are no actions left
> to execute?  e.g. OVS_DROP_NO_MORE_ACTIONS or OVS_DROP_LAST_ACTION
> or OVS_DROP_END_OF_ACTION_LIST or something of that sort?  These may
> seem long, but they are not longer than some other names introduced
> later in the set.  What do yo think?
> 

Hi Ilya,

Thanks for the suggestion. It looks reasonable. I did consider something similar 
but then it felt like having a bit of an "unexpected" or "involuntary" 
connotation. Given that there are other drop reasons that are involutary I 
wanted to somehow differentiate this one from the rest.

Semantically it'd mean something like: When a flow is deliberately installed 
with an empty action list it means "it" (the flow?) _wants_ to drop the packet, 
that's why I ended at that name.

OVS_DROP_LAST_ACTION seems to convey this intentionality as well. I'll can send 
another version changing all names.

Thanks.

-- 
Adrián Moreno