Re: [ovs-dev] Extending ovs_action_attr to add a new action
On Fri, Nov 8, 2019 at 11:04 PM William Tu wrote: > > On Fri, Nov 08, 2019 at 05:12:55PM +0100, Matteo Croce wrote: > > Hi, > > > > I need to add a field to enum ovs_action_attr, but I see that the > > definition between the upstream header[1] and the one in compat[2] > > differs. > > Upstream enum stops at OVS_ACTION_ATTR_CHECK_PKT_LEN, with an extra > > "hidden" element after __OVS_ACTION_ATTR_MAX (22) > > Our compat version instead, has OVS_ACTION_ATTR_TUNNEL_{PUSH,POP} > > defined only #ifndef __KERNEL__, with __OVS_ACTION_ATTR_MAX being 22 > > for the kernel and 24 for userspace. > > > > If I add a field OVS_ACTION_ATTR_WHATEVER just before > > __OVS_ACTION_ATTR_MAX in the kernel, older userspace will incorrectly > > see the new action as OVS_ACTION_ATTR_TUNNEL_PUSH. > > "older userspace" means you're using userspace datapath (dpif-netdev)? > If true, then it's not using kernel module. > > if "older userspace" means just ovs-vswitchd using kernel module, > and you want to upgrade ovs kernel module with your new action > and without upgrade ovs-vswitchd? > Yes, I mean older vswitchd with a new kernel module. If I add a field after OVS_ACTION_ATTR_TUNNEL_POP, and then I downgrade the userspace utils I end up in this situation: # ovs-dpctl dump-flows in_port(1),eth(),eth_type(0x0800), packets:1, bytes:98, used:605.381s, actions:tnl_push(tnl_port(65544),header(size=252,type=131097,eth(dst=14:00:00:00:84:03,src=00:00:03:01:00:00,dl_type=0x0500),ipv6(src=1400::800:1300:0:0:800,dst=200::800:300:200:0:800,label=21504,proto=8,tclass=0x0,hlimit=0),),out_port(2)),2 while adding it before OVS_ACTION_ATTR_TUNNEL_POP I get this: # ovs-dpctl dump-flows in_port(1),eth(),eth_type(0x0800), packets:1, bytes:98, used:3.661s, actions:bad length 0, expected 4 for: action22,2 > Usually we also upgrade ovs-vswitchd, so I don't know how this can be done. > So it's not a problem, we can assume that the userspace can't be older than the kernel datapath. Regards, -- Matteo Croce per aspera ad upstream ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] Extending ovs_action_attr to add a new action
On Fri, Nov 08, 2019 at 05:12:55PM +0100, Matteo Croce wrote: > Hi, > > I need to add a field to enum ovs_action_attr, but I see that the > definition between the upstream header[1] and the one in compat[2] > differs. > Upstream enum stops at OVS_ACTION_ATTR_CHECK_PKT_LEN, with an extra > "hidden" element after __OVS_ACTION_ATTR_MAX (22) > Our compat version instead, has OVS_ACTION_ATTR_TUNNEL_{PUSH,POP} > defined only #ifndef __KERNEL__, with __OVS_ACTION_ATTR_MAX being 22 > for the kernel and 24 for userspace. > > If I add a field OVS_ACTION_ATTR_WHATEVER just before > __OVS_ACTION_ATTR_MAX in the kernel, older userspace will incorrectly > see the new action as OVS_ACTION_ATTR_TUNNEL_PUSH. "older userspace" means you're using userspace datapath (dpif-netdev)? If true, then it's not using kernel module. if "older userspace" means just ovs-vswitchd using kernel module, and you want to upgrade ovs kernel module with your new action and without upgrade ovs-vswitchd? Usually we also upgrade ovs-vswitchd, so I don't know how this can be done. Regards, William > > How can we extend this enum without breaking compatibility? > If OVS_ACTION_ATTR_TUNNEL_* really is unused in the kernel datapath, > what if we pad the kernel header with two padding fields? > > -%<- > --- a/include/uapi/linux/openvswitch.h > +++ b/include/uapi/linux/openvswitch.h > @@ -925,6 +926,8 @@ enum ovs_action_attr { > OVS_ACTION_ATTR_METER,/* u32 meter ID. */ > OVS_ACTION_ATTR_CLONE,/* Nested OVS_CLONE_ATTR_*. */ > OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */ > + _OVS_ACTION_ATTR_TUNNEL_PUSH, /* unused in kernel datapath. */ > + _OVS_ACTION_ATTR_TUNNEL_POP, /* unused in kernel datapath. */ > > __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted > ->%- > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/openvswitch.h?h=v5.3#n899 > [2] > https://github.com/openvswitch/ovs/blob/v2.12.0/datapath/linux/compat/include/linux/openvswitch.h#L962 > > Regards, > -- > Matteo Croce > per aspera ad upstream > > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] Extending ovs_action_attr to add a new action
Bleep bloop. Greetings Matteo Croce, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: fatal: corrupt patch at line 13 Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Extending ovs_action_attr to add a new action The copy of the patch that failed is found in: /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/.git/rebase-apply/patch When you have resolved this problem, run "git am --resolved". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] Extending ovs_action_attr to add a new action
Hi, I need to add a field to enum ovs_action_attr, but I see that the definition between the upstream header[1] and the one in compat[2] differs. Upstream enum stops at OVS_ACTION_ATTR_CHECK_PKT_LEN, with an extra "hidden" element after __OVS_ACTION_ATTR_MAX (22) Our compat version instead, has OVS_ACTION_ATTR_TUNNEL_{PUSH,POP} defined only #ifndef __KERNEL__, with __OVS_ACTION_ATTR_MAX being 22 for the kernel and 24 for userspace. If I add a field OVS_ACTION_ATTR_WHATEVER just before __OVS_ACTION_ATTR_MAX in the kernel, older userspace will incorrectly see the new action as OVS_ACTION_ATTR_TUNNEL_PUSH. How can we extend this enum without breaking compatibility? If OVS_ACTION_ATTR_TUNNEL_* really is unused in the kernel datapath, what if we pad the kernel header with two padding fields? -%<- --- a/include/uapi/linux/openvswitch.h +++ b/include/uapi/linux/openvswitch.h @@ -925,6 +926,8 @@ enum ovs_action_attr { OVS_ACTION_ATTR_METER,/* u32 meter ID. */ OVS_ACTION_ATTR_CLONE,/* Nested OVS_CLONE_ATTR_*. */ OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */ + _OVS_ACTION_ATTR_TUNNEL_PUSH, /* unused in kernel datapath. */ + _OVS_ACTION_ATTR_TUNNEL_POP, /* unused in kernel datapath. */ __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted ->%- [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/openvswitch.h?h=v5.3#n899 [2] https://github.com/openvswitch/ovs/blob/v2.12.0/datapath/linux/compat/include/linux/openvswitch.h#L962 Regards, -- Matteo Croce per aspera ad upstream ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev