Re: [ovs-dev] Extending ovs_action_attr to add a new action

2019-11-09 Thread Matteo Croce
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

2019-11-08 Thread William Tu
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

2019-11-08 Thread 0-day Robot
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