On Tue, Jun 21, 2016 at 07:07:49PM -0700, Jesse Gross wrote:
> On Tue, Jun 21, 2016 at 5:53 PM, Yang, Yi <yi.y.y...@intel.com> wrote:
> > On Tue, Jun 21, 2016 at 09:26:58AM -0700, Jesse Gross wrote:
> >> On Mon, Jun 20, 2016 at 7:30 PM, Yang, Yi <yi.y.y...@intel.com> wrote:
> >> > On Mon, Jun 20, 2016 at 07:00:56PM -0700, Jesse Gross wrote:
> >> >> >>
> >> >> >> Hi, Yi Yang.
> >> >> >>
> >> >> >> Before adding the OVS_VXLAN_EXT_GPE extension to the out-of-tree
> >> >> >> module, you
> >> >> >> should send it to the mainline kernel. Besides, you need a very good
> >> >> >> justification why you can't wait for my patchset to be accepted and
> >> >> >> have
> >> >> >> VXLAN-GPE enabled using rtnetlink.
> >> >> >
> >> >> > Will add VS_VXLAN_EXT_GPE to include/uapi/linux/openvswitch.h and
> >> >> > send a
> >> >> > kernel patch, but ovs and net-next kernel also can work together.
> >> >>
> >> >> I think that you might have misunderstood the gist of the suggestion -
> >> >> please do not add this extension. This is pure compatibility code for
> >> >> existing, old features and should not be extended with new things
> >> >> unless there is a very, very good reason (which would almost certainly
> >> >> be related to fixing bugs, not new functionality) - especially since
> >> >> there's already an existing mechanism to do this.
> >> >
> >> > I'm confused, kernel has this header file
> >> > include/uapi/linux/openvswitch.h, but ovs also has this header file
> >> > datapath/linux/compat/include/linux/openvswitch.h, both of them included
> >> > this enumeration definition:
> >> >
> >> > enum {
> >> > OVS_VXLAN_EXT_UNSPEC,
> >> > OVS_VXLAN_EXT_GBP, /* Flag or __u32 */
> >> > __OVS_VXLAN_EXT_MAX,
> >> > };
> >> >
> >> > Don't we need to make sure they are consistent with the below line?
> >> >
> >> > OVS_VXLAN_EXT_GPE, /* Flag, Generic Protocol Extension */
> >>
> >> You don't need to add this, either upstream or in the OVS tree. It's
> >> already possible to configure VXLAN GPE through the VXLAN netlink
> >> interface, so that's what you should be using.
> >>
> >
> > Here gpe is still an extension to vxlan even if we use rtnetlink to
> > configure it instead of the traditional gbp way, so anyway
> > OVS_VXLAN_EXT_GPE definition is required, now the issue is where we
> > define it. I think the best place is still in
> > ovs/datapath/linux/compat/include/linux/openvswitch.h, it will be better
> > if kernel header file include/uapi/linux/openvswitch.h also has it.
>
> Please take another look at the interface. Going forward, tunnel ports
> are not to be configured through OVS kernel directly but through their
> respective netlink interface (for VXLAN extensions this would mean
> VXLAN netlink) and attached to the datapath. This does not require any
> special modifications to the OVS interface and as a result these
> changes are not necessary.
Hi, Jesse.
The point here is how do we change this code at netdev-vport.c to add GPE
support?
if (!strcmp(type, "vxlan") && !strcmp(ext, "gbp")) {
tnl_cfg.exts |= (1 << OVS_VXLAN_EXT_GBP);
}
I see that we could add another set of flags that only userspace uses and have
some conversion from that to whatever the kernel uses in the compatibility
cases. And maybe that's necessary if we want to reject configurations not
supported by the kernel datapath before we send the command to the kernel.
But, otherwise, I see the point of Yi Yang to add this flag at the headers.
Cascardo.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
