On Wed, Jun 22, 2016 at 5:10 AM, Thadeu Lima de Souza Cascardo
<casca...@redhat.com> wrote:
> 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.
Thanks for clarifying that - it wasn't obvious to me that this was
only referring to userspace.
I think it's better to have a set of userspace flags for this and then
convert to the appropriate kernel format depending on the situation.
It shouldn't be too hard and I really don't want to add things to the
kernel interface that aren't actually used by the kernel.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
