On Thu, 10 Aug 2017 21:21:15 +0800, Yi Yang wrote:
OVS master and 2.8 branch has merged NSH userspace
patch series, this patch is to enable NSH support
in kernel data path in order that OVS can support
NSH in 2.8 release in compat mode by porting this.
Please include changelog when posting a new version of a patch.
I still don't like the "c" name. Please change it to something
descriptive. This is uAPI and can't be changed later.
And I still don't see my comment about this not being extensible for
MD type 2 addressed. Please understand this is uAPI and it is set in
stone once it is merged into the kernel. It's very important we get
this right since the beginning.
This is not how netlink attributes work. Please reread Ben Pfaff's
explanation on how this needs to be structured (Message-ID:
[off-list ref]) and rework the patch. I 100% agree
with what he wrote, his proposal is very clean and matches how netlink
is designed.
A nit: the operation to get/set version, length and flags from the NSH
header seems to be repeated enough to warrant helper functions in
include/net/nsh.h. Something like:
static inline u8 nsh_get_version(const struct nsh_hdr *nsh)
{
return (ntohs(nsh->ver_flags_len) & NSH_VER_MASK) >> NSH_VER_SHIFT;
}
etc.
Not a blocker, though, it may be done later if needed.
quoted hunk
@@ -76,9 +77,11 @@ static bool actions_may_change_flow(const struct nlattr *actions) case OVS_ACTION_ATTR_CT: case OVS_ACTION_ATTR_HASH:+ case OVS_ACTION_ATTR_POP_NSH: case OVS_ACTION_ATTR_POP_ETH: case OVS_ACTION_ATTR_POP_MPLS: case OVS_ACTION_ATTR_POP_VLAN:+ case OVS_ACTION_ATTR_PUSH_NSH: case OVS_ACTION_ATTR_PUSH_ETH: case OVS_ACTION_ATTR_PUSH_MPLS: case OVS_ACTION_ATTR_PUSH_VLAN:
On Fri, Aug 11, 2017 at 10:24:18AM +0200, Jiri Benc wrote:
On Thu, 10 Aug 2017 21:21:15 +0800, Yi Yang wrote:
quoted
OVS master and 2.8 branch has merged NSH userspace
patch series, this patch is to enable NSH support
in kernel data path in order that OVS can support
NSH in 2.8 release in compat mode by porting this.
Please include changelog when posting a new version of a patch.
I still don't like the "c" name. Please change it to something
descriptive. This is uAPI and can't be changed later.
is "__be32 context[4]" ok?
And I still don't see my comment about this not being extensible for
MD type 2 addressed. Please understand this is uAPI and it is set in
stone once it is merged into the kernel. It's very important we get
this right since the beginning.
I understood it, but I don't know how I can do this per your comments,
Per Ben's comments,
PUSH_NSH message looks like
PUSH_NSH begin
nsh base header
MD type 1
PUSH_NSH end
or
PUSH_NSH begin
nsh base header
MD type 2
PUSH_NSH end
If so, I think we don't need struct ovs_action_push_nsh at all, just as
_CT action did, treat it as a variable data buffer.
So define three new netlink attributes
OVS_ACTION_ATTR_NSH_BASE_HEADER
OVS_ACTION_ATTR_NSH_MD1_DATA
OVS_ACTION_ATTR_NSH_MD2_DATA
OVS_ACTION_ATTR_PUSH_NSH is nested netlink attribute, it will nest
OVS_ACTION_ATTR_NSH_BASE_HEADER and OVS_ACTION_ATTR_NSH_MD1_DATA for MD
type 1, it will nest OVS_ACTION_ATTR_NSH_BASE_HEADER and
OVS_ACTION_ATTR_NSH_MD2_DATA for MD type 2. I'll compeletely remove struct
ovs_action_push_nsh, is it ok?
This is not how netlink attributes work. Please reread Ben Pfaff's
explanation on how this needs to be structured (Message-ID:
[off-list ref]) and rework the patch. I 100% agree
with what he wrote, his proposal is very clean and matches how netlink
is designed.
A nit: the operation to get/set version, length and flags from the NSH
header seems to be repeated enough to warrant helper functions in
include/net/nsh.h. Something like:
static inline u8 nsh_get_version(const struct nsh_hdr *nsh)
{
return (ntohs(nsh->ver_flags_len) & NSH_VER_MASK) >> NSH_VER_SHIFT;
}
etc.
Not a blocker, though, it may be done later if needed.
Ok, will change it.
quoted
@@ -76,9 +77,11 @@ static bool actions_may_change_flow(const struct nlattr *actions) case OVS_ACTION_ATTR_CT: case OVS_ACTION_ATTR_HASH:+ case OVS_ACTION_ATTR_POP_NSH: case OVS_ACTION_ATTR_POP_ETH: case OVS_ACTION_ATTR_POP_MPLS: case OVS_ACTION_ATTR_POP_VLAN:+ case OVS_ACTION_ATTR_PUSH_NSH: case OVS_ACTION_ATTR_PUSH_ETH: case OVS_ACTION_ATTR_PUSH_MPLS: case OVS_ACTION_ATTR_PUSH_VLAN:
On Fri, 11 Aug 2017 16:47:23 +0800, Yang, Yi wrote:
is "__be32 context[4]" ok?
Yes, that looks better.
So define three new netlink attributes
OVS_ACTION_ATTR_NSH_BASE_HEADER
OVS_ACTION_ATTR_NSH_MD1_DATA
OVS_ACTION_ATTR_NSH_MD2_DATA
OVS_ACTION_ATTR_PUSH_NSH is nested netlink attribute, it will nest
OVS_ACTION_ATTR_NSH_BASE_HEADER and OVS_ACTION_ATTR_NSH_MD1_DATA for MD
type 1, it will nest OVS_ACTION_ATTR_NSH_BASE_HEADER and
OVS_ACTION_ATTR_NSH_MD2_DATA for MD type 2. I'll compeletely remove struct
ovs_action_push_nsh, is it ok?
Yes, that's the way to do it.
What should be done with struct ovs_key_nsh? Even with "c" renamed to
"context", it's still MD type 1 only structure. What is the plan for
MD type 2 support wrt. this structure?
Thanks,
Jiri
From: Yang, Yi Y <hidden> Date: 2017-08-11 09:24:28
So far, we're not clear how we can support MD type 2 better, as I explained before, we need to reuse tun_metadata in struct flow_tnl which is the thing Geneve is using. Geneve predefined 64 keys for this from tun_metadata0 to tun_metadata63, we will reuse it for MD type 2. But you know NSH is not tunnel, so it has to be changed to support both Geneve and NSH. Anyway, they won't be part of ovs_key_nsh.
-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Jiri Benc
Sent: Friday, August 11, 2017 5:11 PM
To: Yang, Yi Y <redacted>
Cc: netdev@vger.kernel.org; dev@openvswitch.org
Subject: Re: [PATCH net-next v2] openvswitch: enable NSH support
On Fri, 11 Aug 2017 16:47:23 +0800, Yang, Yi wrote:
is "__be32 context[4]" ok?
Yes, that looks better.
So define three new netlink attributes
OVS_ACTION_ATTR_NSH_BASE_HEADER
OVS_ACTION_ATTR_NSH_MD1_DATA
OVS_ACTION_ATTR_NSH_MD2_DATA
OVS_ACTION_ATTR_PUSH_NSH is nested netlink attribute, it will nest
OVS_ACTION_ATTR_NSH_BASE_HEADER and OVS_ACTION_ATTR_NSH_MD1_DATA for
MD type 1, it will nest OVS_ACTION_ATTR_NSH_BASE_HEADER and
OVS_ACTION_ATTR_NSH_MD2_DATA for MD type 2. I'll compeletely remove
struct ovs_action_push_nsh, is it ok?
Yes, that's the way to do it.
What should be done with struct ovs_key_nsh? Even with "c" renamed to "context", it's still MD type 1 only structure. What is the plan for MD type 2 support wrt. this structure?
Thanks,
Jiri
On Fri, 11 Aug 2017 09:24:25 +0000, Yang, Yi Y wrote:
So far, we're not clear how we can support MD type 2 better, as I
explained before, we need to reuse tun_metadata in struct flow_tnl
which is the thing Geneve is using. Geneve predefined 64 keys for
this from tun_metadata0 to tun_metadata63, we will reuse it for MD
type 2. But you know NSH is not tunnel, so it has to be changed to
support both Geneve and NSH. Anyway, they won't be part of
ovs_key_nsh.
Please do not top post.
The context field does not apply to MD type 2. It looks wrong for the
context field to be included in netlink attribute for anything other
than MD type 1. Perhaps it needs to be put into a separate attribute,
too?
Note that I'm talking only about the uAPI. Internally, ovs can use
struct ovs_key_nsh that is MD type 1 only, there's no problem changing
that later. But for the user space interface, this needs to be clean.
This can be solved for example this way:
In include/uapi/linux/openvswitch.h:
struct ovs_key_nsh_base {
__u8 flags;
__u8 mdtype;
__u8 np;
__u8 pad;
__be32 path_hdr;
};
+ one more netlink attribute carrying MD type 1 info. Will probably
require to change OVS_KEY_ATTR_NSH to a nested attribute etc.
In net/openvswitch/flow.h (or perhaps a different header would be more
appropriate?):
struct ovs_key_nsh {
struct ovs_key_nsh_base base;
__be32 context[4];
};
Plus needed conversions between OVS_KEY_ATTR_NSH and struct ovs_key_nsh
when interfacing between the kernel and user space.
That way, we can have MD type 1 support only for now while still being
allowed to redesign things in whatever way later.
Jiri
On Fri, Aug 11, 2017 at 11:44:49AM +0200, Jiri Benc wrote:
On Fri, 11 Aug 2017 09:24:25 +0000, Yang, Yi Y wrote:
quoted
So far, we're not clear how we can support MD type 2 better, as I
explained before, we need to reuse tun_metadata in struct flow_tnl
which is the thing Geneve is using. Geneve predefined 64 keys for
this from tun_metadata0 to tun_metadata63, we will reuse it for MD
type 2. But you know NSH is not tunnel, so it has to be changed to
support both Geneve and NSH. Anyway, they won't be part of
ovs_key_nsh.
Please do not top post.
Sorry for this inconvenience, I'm using outlook. But now I have to use
mutt to address your concern :-)
The context field does not apply to MD type 2. It looks wrong for the
context field to be included in netlink attribute for anything other
than MD type 1. Perhaps it needs to be put into a separate attribute,
too?
Note that I'm talking only about the uAPI. Internally, ovs can use
struct ovs_key_nsh that is MD type 1 only, there's no problem changing
that later. But for the user space interface, this needs to be clean.
This can be solved for example this way:
In include/uapi/linux/openvswitch.h:
struct ovs_key_nsh_base {
__u8 flags;
__u8 mdtype;
__u8 np;
__u8 pad;
__be32 path_hdr;
};
+ one more netlink attribute carrying MD type 1 info. Will probably
require to change OVS_KEY_ATTR_NSH to a nested attribute etc.
In net/openvswitch/flow.h (or perhaps a different header would be more
appropriate?):
struct ovs_key_nsh {
struct ovs_key_nsh_base base;
__be32 context[4];
};
Plus needed conversions between OVS_KEY_ATTR_NSH and struct ovs_key_nsh
when interfacing between the kernel and user space.
That way, we can have MD type 1 support only for now while still being
allowed to redesign things in whatever way later.
From: Jan Scheurich <hidden> Date: 2017-08-11 10:09:36
-----Original Message-----
From: Jiri Benc [mailto:jbenc-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
Sent: Friday, 11 August, 2017 11:45
The context field does not apply to MD type 2. It looks wrong for the
context field to be included in netlink attribute for anything other
than MD type 1. Perhaps it needs to be put into a separate attribute,
too?
Note that I'm talking only about the uAPI. Internally, ovs can use
struct ovs_key_nsh that is MD type 1 only, there's no problem changing
that later. But for the user space interface, this needs to be clean.
This can be solved for example this way:
In include/uapi/linux/openvswitch.h:
struct ovs_key_nsh_base {
__u8 flags;
__u8 mdtype;
__u8 np;
__u8 pad;
__be32 path_hdr;
};
+ one more netlink attribute carrying MD type 1 info. Will probably
require to change OVS_KEY_ATTR_NSH to a nested attribute etc.
In net/openvswitch/flow.h (or perhaps a different header would be more
appropriate?):
struct ovs_key_nsh {
struct ovs_key_nsh_base base;
__be32 context[4];
};
Plus needed conversions between OVS_KEY_ATTR_NSH and struct ovs_key_nsh
when interfacing between the kernel and user space.
That way, we can have MD type 1 support only for now while still being
allowed to redesign things in whatever way later.
Jiri
For the OVS_KEY_ATTR_NSH I agree to move the fixed size MD1 context headers from nsh_base to a separate struct and use nested netlink attributes.
For OVS_ACTION_ATTR_PUSH_NSH attribute any metadata included is opaque to the datapath and should be copied as is into the packet header. I doubt that there is any benefit to model this with nested attributes for MD1 or MD2. This just adds complexity on sender and receiver side and requires updates in case there should be other MD types added later on.
Unless someone can explain to me why the datapath should understand the internal structure/format of metadata in push_nsh, I would strongly vote to keep the metadata as variable length octet sequence in the non-structured OVS_ACTION_ATTR_PUSH_NSH
BR, Jan
Fri, Aug 11, 2017 at 12:09:36PM CEST, jan.scheurich@ericsson.com wrote:
quoted
-----Original Message-----
From: Jiri Benc [mailto:jbenc@redhat.com]
Sent: Friday, 11 August, 2017 11:45
The context field does not apply to MD type 2. It looks wrong for the
context field to be included in netlink attribute for anything other
than MD type 1. Perhaps it needs to be put into a separate attribute,
too?
Note that I'm talking only about the uAPI. Internally, ovs can use
struct ovs_key_nsh that is MD type 1 only, there's no problem changing
that later. But for the user space interface, this needs to be clean.
This can be solved for example this way:
In include/uapi/linux/openvswitch.h:
struct ovs_key_nsh_base {
__u8 flags;
__u8 mdtype;
__u8 np;
__u8 pad;
__be32 path_hdr;
};
+ one more netlink attribute carrying MD type 1 info. Will probably
require to change OVS_KEY_ATTR_NSH to a nested attribute etc.
In net/openvswitch/flow.h (or perhaps a different header would be more
appropriate?):
struct ovs_key_nsh {
struct ovs_key_nsh_base base;
__be32 context[4];
};
Plus needed conversions between OVS_KEY_ATTR_NSH and struct ovs_key_nsh
when interfacing between the kernel and user space.
That way, we can have MD type 1 support only for now while still being
allowed to redesign things in whatever way later.
Jiri
For the OVS_KEY_ATTR_NSH I agree to move the fixed size MD1 context headers from nsh_base to a separate struct and use nested netlink attributes.
For OVS_ACTION_ATTR_PUSH_NSH attribute any metadata included is opaque to the datapath and should be copied as is into the packet header. I doubt that there is any benefit to model this with nested attributes for MD1 or MD2. This just adds complexity on sender and receiver side and requires updates in case there should be other MD types added later on.
Unless someone can explain to me why the datapath should understand the internal structure/format of metadata in push_nsh, I would strongly vote to keep the metadata as variable length octet sequence in the non-structured OVS_ACTION_ATTR_PUSH_NSH
Could you please wrap lines at 72 chars? This is unreadable. Thanks!