On 3 February 2017 at 12:56, Eric Garver <e...@erig.me> wrote:
> On Thu, Feb 02, 2017 at 02:50:01PM -0800, Joe Stringer wrote:
>> On 18 January 2017 at 11:45, Eric Garver <e...@erig.me> wrote:
>> > For out-of-tree datapath, only try genetlink/compat.
>> > For in-tree kernel datapath, try rtnetlink then genetlink.
>> >
>> > Signed-off-by: Eric Garver <e...@erig.me>
>> > ---
>> > lib/dpif-netlink.c | 35 ++++++++++++++++++++++++++++++++---
>> > 1 file changed, 32 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>> > index e6459f358989..769806eadbf1 100644
>> > --- a/lib/dpif-netlink.c
>> > +++ b/lib/dpif-netlink.c
>> > @@ -210,6 +210,11 @@ static int ovs_packet_family;
>> > * Initialized by dpif_netlink_init(). */
>> > static unsigned int ovs_vport_mcgroup;
>> >
>> > +/* rtnetlink information for OVS.
>> > + *
>> > + * Initialized by dpif_netlink_init(). */
>> > +static bool ovs_datapath_out_of_tree = false;
>>
>> Perhaps in the comment briefly mention that if this is true, devices
>> are created using OVS netlink and if it's false devices are created
>> using NETLINK_ROUTE / as LWT?
>
> I'll buff the comment to explain this.
>
>> > +
>> > static int dpif_netlink_init(void);
>> > static int open_dpif(const struct dpif_netlink_dp *, struct dpif **);
>> > static uint32_t dpif_netlink_port_get_pid(const struct dpif *,
>> > @@ -1014,11 +1019,13 @@ dpif_netlink_port_add(struct dpif *dpif_, struct
>> > netdev *netdev,
>> > odp_port_t *port_nop)
>> > {
>> > struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
>> > - int error;
>> > + int error = EOPNOTSUPP;
>> >
>> > fat_rwlock_wrlock(&dpif->upcall_lock);
>> > - error = dpif_netlink_port_create_and_add(dpif, netdev, port_nop);
>> > - if (error == EOPNOTSUPP) {
>> > + if (!ovs_datapath_out_of_tree) {
>> > + error = dpif_netlink_port_create_and_add(dpif, netdev, port_nop);
>> > + }
>> > + if (error) {
>> > error = dpif_netlink_port_add_compat(dpif, netdev, port_nop);
>> > }
>> > fat_rwlock_unlock(&dpif->upcall_lock);
>> > @@ -2503,6 +2510,7 @@ dpif_netlink_init(void)
>> > {
>> > static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
>> > static int error;
>> > + struct netdev *netdev = NULL;
>>
>> This can be in the #ifdef __linux__; otherwise you have an unused
>> variable on other platforms.
>
> Thanks for catching. I'll address that.
>
>> >
>> > if (ovsthread_once_start(&once)) {
>> > error = nl_lookup_genl_family(OVS_DATAPATH_FAMILY,
>> > @@ -2526,6 +2534,27 @@ dpif_netlink_init(void)
>> > error = nl_lookup_genl_mcgroup(OVS_VPORT_FAMILY,
>> > OVS_VPORT_MCGROUP,
>> > &ovs_vport_mcgroup);
>> > }
>> > +#ifdef __linux__
>> > + if (!error) {
>> > + error = netdev_open("ovs-system-probe", "geneve", &netdev);
>> > + if (!error) {
>> > + error = netdev_geneve_create_kind(netdev, "ovs_geneve");
>> > + if (error != EOPNOTSUPP) {
>> > + if (!error) {
>> > + char namebuf[NETDEV_VPORT_NAME_BUFSIZE];
>> > + const char *dp_port;
>> > +
>> > + dp_port = netdev_vport_get_dpif_port(netdev,
>> > namebuf,
>> > + sizeof
>> > namebuf);
>> > + netdev_geneve_destroy(dp_port);
>> > + }
>> > + ovs_datapath_out_of_tree = true;
>> > + }
>> > + netdev_close(netdev);
>> > + error = 0;
>> > + }
>> > + }
>> > +#endif
>>
>> Geneve isn't added yet, so this patch introduces build breakage.
>
> Oops! I rearranged the patches before submitting. I'll move this patch
> to where I originally had it, after adding the newlink support.
>
>> This doesn't look like it matches the most recent discussion on this topic:
>> https://mail.openvswitch.org/pipermail/ovs-dev/2017-January/327476.html
>
> Correct. I opted not to do the LWT probe on init. We have to verify the
> device after creation anyways, which includes checking for LWT. It
> didn't seem like probing for LWT at init gained anything.
OK, thanks for the explanation. I didn't have the full context to
review these patches so was partly going by just checking that the
feedback from the previous version was applied.
If I follow correctly, the backported datapath will register to allow
device creation via NETLINK_ROUTE with the name ovs_foo for foo in
{geneve,gre,vxlan}; in this patch, if creation of such devices is
successful, or if it fails due to any reason other than EOPNOTSUPP,
then we will attempt to use these out of tree tunnel types?
I guess the point is that the backported datapath can register to
allow such device creation, but it may fail since usually it's
configured over OVS genetlink; however, the failure wouldn't be
EOPNOTSUPP, it'd be something like EINVAL which still tells us that
the ovs_foo modules exist and therefore they should be used (but
configured over genetlink).
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
