On 28 Aug 2024, at 15:55, Aaron Conole wrote:
> Eelco Chaudron <[email protected]> writes:
>
>> Added new actions to the get_ovs_action_attr_str() function.
>
> We should also note that the DROP attribute changed from a userspace
> only action - it means that there could be different results depending
> on which script and OVS version are being used. Might need a NEWS or
> other disclaimer to make users aware. I didn't find a documented
> requirement that usdt scripts need to be paired with like versions of
> the ovs-vswitchd (although, we probably treat it as an implicit
> requirement).
We discussed this before and the USDT scripts are for reference only, and work
with the current version of the tree. Thats is the reason why with RHEL/Fedora
we ship them in the version-specific test package.
>> Fixes: 8bb065961e54 ("dpif: Stub out unimplemented action
>> OVS_ACTION_ATTR_DEC_TTL.")
>> Fixes: 3c8d069b9bc2 ("dpif: Probe support for OVS_ACTION_ATTR_DROP.")
>
> NOTE for MAINTAINERS: if we do backport this fix, we need to be careful
> around this change in particular as there was some netlink changes
> related to this.
Agreed, we should only backport this to 3.4 as these changes do not exists in
earlier branches.
>> Fixes: 1a3bd96b4fc4 ("odp-util: Add support OVS_ACTION_ATTR_PSAMPLE.")
>> Signed-off-by: Eelco Chaudron <[email protected]>
>> ---
>
> The string change is fine, but for the future we should consider
> switching to pahole to populate the ovs_action_attr string.
>
> ex:
>
> ~/git/ovs$ pahole ./vswitchd/ovs-vswitchd -C ovs_action_attr
> enum ovs_action_attr {
> OVS_ACTION_ATTR_UNSPEC = 0,
> OVS_ACTION_ATTR_OUTPUT = 1,
> OVS_ACTION_ATTR_USERSPACE = 2,
> OVS_ACTION_ATTR_SET = 3,
> OVS_ACTION_ATTR_PUSH_VLAN = 4,
> OVS_ACTION_ATTR_POP_VLAN = 5,
> OVS_ACTION_ATTR_SAMPLE = 6,
> OVS_ACTION_ATTR_RECIRC = 7,
> OVS_ACTION_ATTR_HASH = 8,
> OVS_ACTION_ATTR_PUSH_MPLS = 9,
> OVS_ACTION_ATTR_POP_MPLS = 10,
> OVS_ACTION_ATTR_SET_MASKED = 11,
> OVS_ACTION_ATTR_CT = 12,
> OVS_ACTION_ATTR_TRUNC = 13,
> OVS_ACTION_ATTR_PUSH_ETH = 14,
> OVS_ACTION_ATTR_POP_ETH = 15,
> OVS_ACTION_ATTR_CT_CLEAR = 16,
> OVS_ACTION_ATTR_PUSH_NSH = 17,
> OVS_ACTION_ATTR_POP_NSH = 18,
> OVS_ACTION_ATTR_METER = 19,
> OVS_ACTION_ATTR_CLONE = 20,
> OVS_ACTION_ATTR_CHECK_PKT_LEN = 21,
> OVS_ACTION_ATTR_ADD_MPLS = 22,
> OVS_ACTION_ATTR_DEC_TTL = 23,
> OVS_ACTION_ATTR_DROP = 24,
> OVS_ACTION_ATTR_TUNNEL_PUSH = 25,
> OVS_ACTION_ATTR_TUNNEL_POP = 26,
> OVS_ACTION_ATTR_LB_OUTPUT = 27,
> __OVS_ACTION_ATTR_MAX = 28,
> };
>
> So we could do something like (untested):
>
> ovs_action_attr = get_ovs_definitions("ovs_action_attr", ...)
>
> Such a change would be a separate patch, as I think we can accept this
> to get the enums properly in line, but the end goal should be to
> eliminate all the maintenance burden on keeping these enums around.
ACK, yes we should switch to something like this. I have an item on my plate to
create a library with all kinds of helper functions :)
For now, we should leave it as is, as in the current state it has no
dependencies on pahole.
>> utilities/usdt-scripts/dpif_nl_exec_monitor.py | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/utilities/usdt-scripts/dpif_nl_exec_monitor.py
>> b/utilities/usdt-scripts/dpif_nl_exec_monitor.py
>> index 0a9ff8123..a5cf1a35a 100755
>> --- a/utilities/usdt-scripts/dpif_nl_exec_monitor.py
>> +++ b/utilities/usdt-scripts/dpif_nl_exec_monitor.py
>> @@ -468,9 +468,11 @@ def get_ovs_action_attr_str(attr):
>> "OVS_ACTION_ATTR_CLONE",
>> "OVS_ACTION_ATTR_CHECK_PKT_LEN",
>> "OVS_ACTION_ATTR_ADD_MPLS",
>> + "OVS_ACTION_ATTR_DEC_TTL",
>> + "OVS_ACTION_ATTR_DROP",
>> + "OVS_ACTION_ATTR_PSAMPLE",
>> "OVS_ACTION_ATTR_TUNNEL_PUSH",
>> "OVS_ACTION_ATTR_TUNNEL_POP",
>> - "OVS_ACTION_ATTR_DROP",
>> "OVS_ACTION_ATTR_LB_OUTPUT"]
>> if attr < 0 or attr >= len(ovs_action_attr):
>> return "<UNKNOWN:{}>".format(attr)
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
