Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ilya Maximets]

On 22.11.2019 14:51, Ophir Munk wrote:
> Hi Ilya,
> Thanks for your explanations.
> We are using just one vport ("vxlan0") and still the netdev flow 
> api pointer is NULL during a flow_put() call.
> Following is a root cause description.
> 
> In short: 
> 1. A new port->netdev named "vxlan_sys_4789" is created as part of adding the
> "vxlan0" netdev port. 
> The new port->netdev is created without initializing it with flow api 
> pointers.
> 2. We call netdev_init_flow_api(netdev) on the "vxlan0" netdev. 
> 3. During flow_put we call with the port->netdev "vxlan_sys_4789", whose flow
> api pointers are NULL.  
> 
> In details:
> 1. For "vxlan0" interface we have a call dpif_port_add (dpif.c)
> 2. Inside this function we have a call
> dpif->dpif_class->port_add(dpif, netdev, &port_no);
> Which calls the netdev API  dpif_netdev_port_add()
> 3. Inside dpif_netdev_port_add() dpif_port gets a new name: "vxlan_sys_4789":
> dpif_port = netdev_vport_get_dpif_port(netdev, namebuf, sizeof namebuf);
> 
> Then we call do_add_port(dp, dpif_port, netdev_get_type(netdev), port_no) with
> the new name.
> We have a sequence of calls that eventually creates a new netdev for 
> dpif_port "vxlan_sys_4789"
> do_add_port(dp, dpif_port, netdev_get_type(netdev), port_no);
>==> port_create(devname, type, port_no, &port);
>==> netdev_open(devname, type, &netdev);
> ==> netdev = rc->class->alloc();
> The newly created port holds the new port->netdev and it is added to hash map:
> hmap_insert(&dp->ports, &port->node, hash_port_no(port_no));
> Please note port->netdev was not initialized with flow api.   
>   
> Back to dpif_port_add (dpif.c) we then have a call at the end to 
> netdev_ports_insert() (file netdev_offload.c) which calls 
> netdev_init_flow_api("vxlan0" netdev). 
> The netdev parameter is the "vxlan0" netdev and not the port->netdev 
> "vxlan_sys_4789"
> that was created.
> 
> When dp_netdev_flow_add() it called (file dpif-netdev.c) we will find 
> the port->netdev "vxlan_sys_4789". 
> As mentioned this netdev has never been initialized with flow API. 
> Therefore its flow pointers are NULL.
> The pointers were set successfully on the "vxlan0" netdev - but these pointer 
> are not
> used during a flow_put() call.
> 
> By the way for a "dpdk" type netdev this issue does not happen. It is because 
> the dpif port
> name has the same name as the interface name. 
> No new netdev is allocated. Therefore flow_put will call with the 
> port->netdev that was
> initialized with flow api pointers.
> Hence there is no issue in this case.
> 
> I would appreciate your comments on this.
> 

Thanks for figuring this out!

Yes, it seems a bit strange that dpif-netdev creates new netdev instance
and maybe we could avoid that somehow.  However, this unveiled the bug
which is improper usage of offloading API.  Datapath should request
netdevs from the netdev-offload module with netdev_ports_get() and use
resulted netdev for netdev_flow_put(), but dpif-netdev for some reason
performs lookup in the datapath port list and that is the issue.

I prepared the patch for that:
https://patchwork.ozlabs.org/patch/1199525/

Some changes will be needed to use it along with my previous patch-set
for vport offloading, but they are straightforward.

Best regards, Ilya Maximets.

> Regards,
> Ophir
> 
>> -Original Message-
>> From: Ilya Maximets 
>> Sent: Wednesday, November 20, 2019 7:00 PM
>> To: Ophir Munk ; Ilya Maximets
>> ; ovs-dev@openvswitch.org
>> Cc: Ameer Mahagneh ; Roni Bar Yanai
>> ; Eveline Raine ; Oz
>> Shlomo ; Eli Britstein 
>> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
>>
>> On 20.11.2019 17:52, Ophir Munk wrote:
>>> Hi Ilya,
>>> I think the problem is that the newly created netdev called
>> "vxlan_sys_4789" never goes through a netdev_init_flow_api(netdev); call.
>>
>> 'vxlan_sys_4789' is not a netdev. It's a dpif_port.
>> There is a corresponding 'netdev', but only 'ofproto' knows which one.
>>
>>> Where do you suggest adding this call?
>>>
>>> Regards,
>>> Ophir
>>>
 -Original Message-
 From: Ilya Maximets 
 Sent: Tuesday, November 19, 2019 8:30 PM
 To: Ophir Munk ; Ilya Maximets
 ; ovs-dev@openvswitch.org
 Cc: Ameer Mahagneh ; Roni Bar Yanai
 ; Eveline Raine ; Oz
 Shlomo ; Eli Britstein 
 Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.

 On 19.11.2019 18:20, Ophir Munk wrote:
> Hi Ilya,
> Thanks for the patch set which adds the dpif hint inside the netdev
>> struct.
 It is really helpful.
> Our goal is to have flow_put() calls on vxlan devices, using the
> existing dpdk
 flow API.
> We added logic inside netdev_dpdk_flow_api_supported() to accept
> vxlan devices and indeed we see in the log:
> "netdev_offload|INFO|vxlan0: Assigned flow API 'dpdk_flow_api'".
> However, there is no flow_put() on the vxlan device.
> We see our own printouts in dpif-netdev such a

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ophir Munk]

Hi Ilya,
Thanks for your explanations.
We are using just one vport ("vxlan0") and still the netdev flow 
api pointer is NULL during a flow_put() call.
Following is a root cause description.

In short: 
1. A new port->netdev named "vxlan_sys_4789" is created as part of adding the
"vxlan0" netdev port. 
The new port->netdev is created without initializing it with flow api pointers.
2. We call netdev_init_flow_api(netdev) on the "vxlan0" netdev. 
3. During flow_put we call with the port->netdev "vxlan_sys_4789", whose flow
api pointers are NULL.  

In details:
1. For "vxlan0" interface we have a call dpif_port_add (dpif.c)
2. Inside this function we have a call
dpif->dpif_class->port_add(dpif, netdev, &port_no);
Which calls the netdev API  dpif_netdev_port_add()
3. Inside dpif_netdev_port_add() dpif_port gets a new name: "vxlan_sys_4789":
dpif_port = netdev_vport_get_dpif_port(netdev, namebuf, sizeof namebuf);

Then we call do_add_port(dp, dpif_port, netdev_get_type(netdev), port_no) with
the new name.
We have a sequence of calls that eventually creates a new netdev for 
dpif_port "vxlan_sys_4789"
do_add_port(dp, dpif_port, netdev_get_type(netdev), port_no);
   ==> port_create(devname, type, port_no, &port);
   ==> netdev_open(devname, type, &netdev);
  ==> netdev = rc->class->alloc();
The newly created port holds the new port->netdev and it is added to hash map:
hmap_insert(&dp->ports, &port->node, hash_port_no(port_no));
Please note port->netdev was not initialized with flow api. 

Back to dpif_port_add (dpif.c) we then have a call at the end to 
netdev_ports_insert() (file netdev_offload.c) which calls 
netdev_init_flow_api("vxlan0" netdev). 
The netdev parameter is the "vxlan0" netdev and not the port->netdev 
"vxlan_sys_4789"
that was created.

When dp_netdev_flow_add() it called (file dpif-netdev.c) we will find 
the port->netdev "vxlan_sys_4789". 
As mentioned this netdev has never been initialized with flow API. 
Therefore its flow pointers are NULL.
The pointers were set successfully on the "vxlan0" netdev - but these pointer 
are not
used during a flow_put() call.

By the way for a "dpdk" type netdev this issue does not happen. It is because 
the dpif port
name has the same name as the interface name. 
No new netdev is allocated. Therefore flow_put will call with the port->netdev 
that was
initialized with flow api pointers.
Hence there is no issue in this case.

I would appreciate your comments on this.

Regards,
Ophir

> -Original Message-
> From: Ilya Maximets 
> Sent: Wednesday, November 20, 2019 7:00 PM
> To: Ophir Munk ; Ilya Maximets
> ; ovs-dev@openvswitch.org
> Cc: Ameer Mahagneh ; Roni Bar Yanai
> ; Eveline Raine ; Oz
> Shlomo ; Eli Britstein 
> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
> 
> On 20.11.2019 17:52, Ophir Munk wrote:
> > Hi Ilya,
> > I think the problem is that the newly created netdev called
> "vxlan_sys_4789" never goes through a netdev_init_flow_api(netdev); call.
> 
> 'vxlan_sys_4789' is not a netdev. It's a dpif_port.
> There is a corresponding 'netdev', but only 'ofproto' knows which one.
> 
> > Where do you suggest adding this call?
> >
> > Regards,
> > Ophir
> >
> >> -Original Message-
> >> From: Ilya Maximets 
> >> Sent: Tuesday, November 19, 2019 8:30 PM
> >> To: Ophir Munk ; Ilya Maximets
> >> ; ovs-dev@openvswitch.org
> >> Cc: Ameer Mahagneh ; Roni Bar Yanai
> >> ; Eveline Raine ; Oz
> >> Shlomo ; Eli Britstein 
> >> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
> >>
> >> On 19.11.2019 18:20, Ophir Munk wrote:
> >>> Hi Ilya,
> >>> Thanks for the patch set which adds the dpif hint inside the netdev
> struct.
> >> It is really helpful.
> >>> Our goal is to have flow_put() calls on vxlan devices, using the
> >>> existing dpdk
> >> flow API.
> >>> We added logic inside netdev_dpdk_flow_api_supported() to accept
> >>> vxlan devices and indeed we see in the log:
> >>> "netdev_offload|INFO|vxlan0: Assigned flow API 'dpdk_flow_api'".
> >>> However, there is no flow_put() on the vxlan device.
> >>> We see our own printouts in dpif-netdev such as:
> >>> "dpif_netdev(dp_netdev_flow_5)|ERR|vxlan_sys_4789: calling netdev
> >> flow_put()"
> >>> but inside netdev_flow_put the flow_api poiner is NULL.
> >>> Any ideas why?
> >>
> >> How many tunneling ports you have?
> >> The case here is that dpif creates only one dpif_port for all the
> >> tunnels with similar config.  So, only one of these netdevs will have
> >> flow api initialized and you need to be sure that you're using the
> >> right one.  You may find it by the datapath 'port_no' with
> netdev_ports_get().
> >> Just guessing, but this is the one possible reason.
> >>
> >>>
>  -Original Message-
>  From: Ilya Maximets 
>  Sent: Friday, November 15, 2019 10:26 PM
>  To: Ophir Munk ; Ilya Maximets
>  ; ovs-dev@openvswitch.org
>  Cc: Ameer Mahagneh ; Roni Bar Yanai
>  ; Eveline Raine ; Oz
>  Shlomo ; 

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ophir Munk]

Hi Ilya,
I think the problem is that the newly created netdev called "vxlan_sys_4789" 
never goes through a netdev_init_flow_api(netdev); call.
Where do you suggest adding this call?

Regards,
Ophir

> -Original Message-
> From: Ilya Maximets 
> Sent: Tuesday, November 19, 2019 8:30 PM
> To: Ophir Munk ; Ilya Maximets
> ; ovs-dev@openvswitch.org
> Cc: Ameer Mahagneh ; Roni Bar Yanai
> ; Eveline Raine ; Oz
> Shlomo ; Eli Britstein 
> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
> 
> On 19.11.2019 18:20, Ophir Munk wrote:
> > Hi Ilya,
> > Thanks for the patch set which adds the dpif hint inside the netdev struct.
> It is really helpful.
> > Our goal is to have flow_put() calls on vxlan devices, using the existing 
> > dpdk
> flow API.
> > We added logic inside netdev_dpdk_flow_api_supported() to accept vxlan
> > devices and indeed we see in the log:
> > "netdev_offload|INFO|vxlan0: Assigned flow API 'dpdk_flow_api'".
> > However, there is no flow_put() on the vxlan device.
> > We see our own printouts in dpif-netdev such as:
> > "dpif_netdev(dp_netdev_flow_5)|ERR|vxlan_sys_4789: calling netdev
> flow_put()"
> > but inside netdev_flow_put the flow_api poiner is NULL.
> > Any ideas why?
> 
> How many tunneling ports you have?
> The case here is that dpif creates only one dpif_port for all the tunnels with
> similar config.  So, only one of these netdevs will have flow api initialized 
> and
> you need to be sure that you're using the right one.  You may find it by the
> datapath 'port_no' with netdev_ports_get().
> Just guessing, but this is the one possible reason.
> 
> >
> >> -Original Message-
> >> From: Ilya Maximets 
> >> Sent: Friday, November 15, 2019 10:26 PM
> >> To: Ophir Munk ; Ilya Maximets
> >> ; ovs-dev@openvswitch.org
> >> Cc: Ameer Mahagneh ; Roni Bar Yanai
> >> ; Eveline Raine ; Oz
> >> Shlomo ; Eli Britstein 
> >> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
> >>
> >> On 10.11.2019 21:17, Ophir Munk wrote:
> >>> Hi Ilya,
> >>> Thanks you for taking care of this.
> >>> Assigning the correct vport flow API is a critical feature for offloading.
> >>>
> >>> It seems hard to address all different vport configuration scenarios
> >>> (kernel
> >> only, userspace only or both) by just relying on the individual
> >> netdev and without knowing the dpif on top.
> >>
> >> Yes you're right.  The only module that knows the real mapping of
> >> 'netdev's to 'dpif's is 'ofproto' and we need to get this information
> somehow.
> >>
> >>> Maybe can move the flow API assignment to the point where dp is
> >>> actually
> >> adding the port netdev and give a hint about the dp.
> >>
> >> Since we already have a hint from the upper layers about the
> >> 'dpif_class' I'm suggesting to replace it with 'dpif_type'.
> >> In pair with storing this hint inside the netdev we could have a
> >> flexible system without exposing internals to higher layers or trying
> >> to use higher layer from the library code.
> >>
> >> Please, take a look at the new version of patches:
> >>
> >>
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.
> >> openvswitch.org%2Fpipermail%2Fovs-dev%2F2019-
> >>
> November%2F364735.html&data=02%7C01%7Cophirmu%40mellanox.c
> >>
> om%7Caba8a2cdff2342aa536b08d76a09fc61%7Ca652971c7d2e4d9ba6a4d149
> >>
> 256f461b%7C0%7C0%7C637094463434958586&sdata=taEV4B7BA83%2Fu
> >> faHoW7Y7F824SumWEnEXhVUpUON5eA%3D&reserved=0
> >>
> >>> It is also confusing that userspace vport names are "sys_vxlan_4789"
> >>> and
> >> not "usr_vxlan_4789" for example.
> >>
> >> Yes, this is a bit confusing, but we, probably, could not just change
> >> it.  At least that we'll have to rewrite a big part of system tests
> >> that relies on the port names in the flow dumps.
> >> We will have to duplicate them for kernel and userspace.
> >> Some external management/monitoring tools could be affected too
> >> because they will have to treat kernel and userspace tunneling
> differently.
> >>
> >> Best regards, Ilya Maximets.
> >>
> >>> What do you think?
> >>>
> >>> Regards,
> >>> Ophir
> >>>
>  -Original Message-
>  From: Ilya Maximets 
>  Sent: Friday, November 8, 2019 5:57 PM
>  To: Ophir Munk ; Ilya Maximets
>  ; ovs-dev@openvswitch.org
>  Cc: Ameer Mahagneh ; Roni Bar Yanai
>  ; Eveline Raine 
>  Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
> 
>  On 06.11.2019 18:11, Ophir Munk wrote:
> > Hi Ilya,
> > A few months ago we discussed the missing functionality of vports
>  offloading under user space bridges.
> > Commit [1] was added to explicitly avoid installing userspace
> > vport flows (to
>  avoid confusion with the vport kernel).
> > When reverting commit [1] - we are left with this missing
> functionality.
> > Applying your suggested patch netdev_vport_has_system_port()
> API)
>  does not seem to work.
> > It always fails when it tries to look for the interface name (say
> >

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ilya Maximets]

On 20.11.2019 17:52, Ophir Munk wrote:
> Hi Ilya,
> I think the problem is that the newly created netdev called "vxlan_sys_4789" 
> never goes through a netdev_init_flow_api(netdev); call.

'vxlan_sys_4789' is not a netdev. It's a dpif_port.
There is a corresponding 'netdev', but only 'ofproto' knows which one.

> Where do you suggest adding this call?
> 
> Regards,
> Ophir
> 
>> -Original Message-
>> From: Ilya Maximets 
>> Sent: Tuesday, November 19, 2019 8:30 PM
>> To: Ophir Munk ; Ilya Maximets
>> ; ovs-dev@openvswitch.org
>> Cc: Ameer Mahagneh ; Roni Bar Yanai
>> ; Eveline Raine ; Oz
>> Shlomo ; Eli Britstein 
>> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
>>
>> On 19.11.2019 18:20, Ophir Munk wrote:
>>> Hi Ilya,
>>> Thanks for the patch set which adds the dpif hint inside the netdev struct.
>> It is really helpful.
>>> Our goal is to have flow_put() calls on vxlan devices, using the existing 
>>> dpdk
>> flow API.
>>> We added logic inside netdev_dpdk_flow_api_supported() to accept vxlan
>>> devices and indeed we see in the log:
>>> "netdev_offload|INFO|vxlan0: Assigned flow API 'dpdk_flow_api'".
>>> However, there is no flow_put() on the vxlan device.
>>> We see our own printouts in dpif-netdev such as:
>>> "dpif_netdev(dp_netdev_flow_5)|ERR|vxlan_sys_4789: calling netdev
>> flow_put()"
>>> but inside netdev_flow_put the flow_api poiner is NULL.
>>> Any ideas why?
>>
>> How many tunneling ports you have?
>> The case here is that dpif creates only one dpif_port for all the tunnels 
>> with
>> similar config.  So, only one of these netdevs will have flow api 
>> initialized and
>> you need to be sure that you're using the right one.  You may find it by the
>> datapath 'port_no' with netdev_ports_get().
>> Just guessing, but this is the one possible reason.
>>
>>>
 -Original Message-
 From: Ilya Maximets 
 Sent: Friday, November 15, 2019 10:26 PM
 To: Ophir Munk ; Ilya Maximets
 ; ovs-dev@openvswitch.org
 Cc: Ameer Mahagneh ; Roni Bar Yanai
 ; Eveline Raine ; Oz
 Shlomo ; Eli Britstein 
 Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.

 On 10.11.2019 21:17, Ophir Munk wrote:
> Hi Ilya,
> Thanks you for taking care of this.
> Assigning the correct vport flow API is a critical feature for offloading.
>
> It seems hard to address all different vport configuration scenarios
> (kernel
 only, userspace only or both) by just relying on the individual
 netdev and without knowing the dpif on top.

 Yes you're right.  The only module that knows the real mapping of
 'netdev's to 'dpif's is 'ofproto' and we need to get this information
>> somehow.

> Maybe can move the flow API assignment to the point where dp is
> actually
 adding the port netdev and give a hint about the dp.

 Since we already have a hint from the upper layers about the
 'dpif_class' I'm suggesting to replace it with 'dpif_type'.
 In pair with storing this hint inside the netdev we could have a
 flexible system without exposing internals to higher layers or trying
 to use higher layer from the library code.

 Please, take a look at the new version of patches:


>> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.
 openvswitch.org%2Fpipermail%2Fovs-dev%2F2019-

>> November%2F364735.html&data=02%7C01%7Cophirmu%40mellanox.c

>> om%7Caba8a2cdff2342aa536b08d76a09fc61%7Ca652971c7d2e4d9ba6a4d149

>> 256f461b%7C0%7C0%7C637094463434958586&sdata=taEV4B7BA83%2Fu
 faHoW7Y7F824SumWEnEXhVUpUON5eA%3D&reserved=0

> It is also confusing that userspace vport names are "sys_vxlan_4789"
> and
 not "usr_vxlan_4789" for example.

 Yes, this is a bit confusing, but we, probably, could not just change
 it.  At least that we'll have to rewrite a big part of system tests
 that relies on the port names in the flow dumps.
 We will have to duplicate them for kernel and userspace.
 Some external management/monitoring tools could be affected too
 because they will have to treat kernel and userspace tunneling
>> differently.

 Best regards, Ilya Maximets.

> What do you think?
>
> Regards,
> Ophir
>
>> -Original Message-
>> From: Ilya Maximets 
>> Sent: Friday, November 8, 2019 5:57 PM
>> To: Ophir Munk ; Ilya Maximets
>> ; ovs-dev@openvswitch.org
>> Cc: Ameer Mahagneh ; Roni Bar Yanai
>> ; Eveline Raine 
>> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
>>
>> On 06.11.2019 18:11, Ophir Munk wrote:
>>> Hi Ilya,
>>> A few months ago we discussed the missing functionality of vports
>> offloading under user space bridges.
>>> Commit [1] was added to explicitly avoid installing userspace
>>> vport flows (to
>> avoid confusion with the vport kernel).
>>> When reverting commit [1] - we are left with this mi

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ilya Maximets]

On 19.11.2019 18:20, Ophir Munk wrote:
> Hi Ilya,
> Thanks for the patch set which adds the dpif hint inside the netdev struct. 
> It is really helpful.
> Our goal is to have flow_put() calls on vxlan devices, using the existing 
> dpdk flow API.
> We added logic inside netdev_dpdk_flow_api_supported() to accept vxlan 
> devices and indeed 
> we see in the log: 
> "netdev_offload|INFO|vxlan0: Assigned flow API 'dpdk_flow_api'".
> However, there is no flow_put() on the vxlan device. 
> We see our own printouts in dpif-netdev such as:
> "dpif_netdev(dp_netdev_flow_5)|ERR|vxlan_sys_4789: calling netdev flow_put()" 
> but inside netdev_flow_put the flow_api poiner is NULL.
> Any ideas why?

How many tunneling ports you have?
The case here is that dpif creates only one dpif_port for all the
tunnels with similar config.  So, only one of these netdevs will have
flow api initialized and you need to be sure that you're using the
right one.  You may find it by the datapath 'port_no' with
netdev_ports_get().
Just guessing, but this is the one possible reason.

> 
>> -Original Message-
>> From: Ilya Maximets 
>> Sent: Friday, November 15, 2019 10:26 PM
>> To: Ophir Munk ; Ilya Maximets
>> ; ovs-dev@openvswitch.org
>> Cc: Ameer Mahagneh ; Roni Bar Yanai
>> ; Eveline Raine ; Oz
>> Shlomo ; Eli Britstein 
>> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
>>
>> On 10.11.2019 21:17, Ophir Munk wrote:
>>> Hi Ilya,
>>> Thanks you for taking care of this.
>>> Assigning the correct vport flow API is a critical feature for offloading.
>>>
>>> It seems hard to address all different vport configuration scenarios (kernel
>> only, userspace only or both) by just relying on the individual netdev and
>> without knowing the dpif on top.
>>
>> Yes you're right.  The only module that knows the real mapping of 'netdev's
>> to 'dpif's is 'ofproto' and we need to get this information somehow.
>>
>>> Maybe can move the flow API assignment to the point where dp is actually
>> adding the port netdev and give a hint about the dp.
>>
>> Since we already have a hint from the upper layers about the 'dpif_class' I'm
>> suggesting to replace it with 'dpif_type'.
>> In pair with storing this hint inside the netdev we could have a flexible
>> system without exposing internals to higher layers or trying to use higher
>> layer from the library code.
>>
>> Please, take a look at the new version of patches:
>>
>> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.
>> openvswitch.org%2Fpipermail%2Fovs-dev%2F2019-
>> November%2F364735.html&data=02%7C01%7Cophirmu%40mellanox.c
>> om%7Caba8a2cdff2342aa536b08d76a09fc61%7Ca652971c7d2e4d9ba6a4d149
>> 256f461b%7C0%7C0%7C637094463434958586&sdata=taEV4B7BA83%2Fu
>> faHoW7Y7F824SumWEnEXhVUpUON5eA%3D&reserved=0
>>
>>> It is also confusing that userspace vport names are "sys_vxlan_4789" and
>> not "usr_vxlan_4789" for example.
>>
>> Yes, this is a bit confusing, but we, probably, could not just change it.  
>> At least
>> that we'll have to rewrite a big part of system tests that relies on the port
>> names in the flow dumps.
>> We will have to duplicate them for kernel and userspace.
>> Some external management/monitoring tools could be affected too because
>> they will have to treat kernel and userspace tunneling differently.
>>
>> Best regards, Ilya Maximets.
>>
>>> What do you think?
>>>
>>> Regards,
>>> Ophir
>>>
 -Original Message-
 From: Ilya Maximets 
 Sent: Friday, November 8, 2019 5:57 PM
 To: Ophir Munk ; Ilya Maximets
 ; ovs-dev@openvswitch.org
 Cc: Ameer Mahagneh ; Roni Bar Yanai
 ; Eveline Raine 
 Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.

 On 06.11.2019 18:11, Ophir Munk wrote:
> Hi Ilya,
> A few months ago we discussed the missing functionality of vports
 offloading under user space bridges.
> Commit [1] was added to explicitly avoid installing userspace vport
> flows (to
 avoid confusion with the vport kernel).
> When reverting commit [1] - we are left with this missing functionality.
> Applying your suggested patch netdev_vport_has_system_port() API)
 does not seem to work.
> It always fails when it tries to look for the interface name (say
> "vxlan10") in
 the system list where vxlan interfaces are renamed (say
>> "vxlan_sys_4789").

 Hi Ophir,

 No-one ever tried to run this code, so I'm not surprised.
 I could take a look at this issue on next week.

 Best regards, Ilya Maximets.

>
> You wrote:
>> On master, after applying dynamic flow API patch-set, we'll be able
>> to use patch that I suggested in previous mail to properly handle
>> this situation and enable vport offloading.
>
> It would be appreciated if we can resume the efforts in fixing this
> missing
 functionality.
> Please advise on the next steps.
>
> [1]
> Commit 0da667e345 ("dpif-netdev: Forbid vport offloa

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ophir Munk]

Hi Ilya,
Thanks for the patch set which adds the dpif hint inside the netdev struct. It 
is really helpful.
Our goal is to have flow_put() calls on vxlan devices, using the existing dpdk 
flow API.
We added logic inside netdev_dpdk_flow_api_supported() to accept vxlan devices 
and indeed 
we see in the log: 
"netdev_offload|INFO|vxlan0: Assigned flow API 'dpdk_flow_api'".
However, there is no flow_put() on the vxlan device. 
We see our own printouts in dpif-netdev such as:
"dpif_netdev(dp_netdev_flow_5)|ERR|vxlan_sys_4789: calling netdev flow_put()" 
but inside netdev_flow_put the flow_api poiner is NULL.
Any ideas why?

Regards,
Ophir

> -Original Message-
> From: Ilya Maximets 
> Sent: Friday, November 15, 2019 10:26 PM
> To: Ophir Munk ; Ilya Maximets
> ; ovs-dev@openvswitch.org
> Cc: Ameer Mahagneh ; Roni Bar Yanai
> ; Eveline Raine ; Oz
> Shlomo ; Eli Britstein 
> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
> 
> On 10.11.2019 21:17, Ophir Munk wrote:
> > Hi Ilya,
> > Thanks you for taking care of this.
> > Assigning the correct vport flow API is a critical feature for offloading.
> >
> > It seems hard to address all different vport configuration scenarios (kernel
> only, userspace only or both) by just relying on the individual netdev and
> without knowing the dpif on top.
> 
> Yes you're right.  The only module that knows the real mapping of 'netdev's
> to 'dpif's is 'ofproto' and we need to get this information somehow.
> 
> > Maybe can move the flow API assignment to the point where dp is actually
> adding the port netdev and give a hint about the dp.
> 
> Since we already have a hint from the upper layers about the 'dpif_class' I'm
> suggesting to replace it with 'dpif_type'.
> In pair with storing this hint inside the netdev we could have a flexible
> system without exposing internals to higher layers or trying to use higher
> layer from the library code.
> 
> Please, take a look at the new version of patches:
> 
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.
> openvswitch.org%2Fpipermail%2Fovs-dev%2F2019-
> November%2F364735.html&data=02%7C01%7Cophirmu%40mellanox.c
> om%7Caba8a2cdff2342aa536b08d76a09fc61%7Ca652971c7d2e4d9ba6a4d149
> 256f461b%7C0%7C0%7C637094463434958586&sdata=taEV4B7BA83%2Fu
> faHoW7Y7F824SumWEnEXhVUpUON5eA%3D&reserved=0
> 
> > It is also confusing that userspace vport names are "sys_vxlan_4789" and
> not "usr_vxlan_4789" for example.
> 
> Yes, this is a bit confusing, but we, probably, could not just change it.  At 
> least
> that we'll have to rewrite a big part of system tests that relies on the port
> names in the flow dumps.
> We will have to duplicate them for kernel and userspace.
> Some external management/monitoring tools could be affected too because
> they will have to treat kernel and userspace tunneling differently.
> 
> Best regards, Ilya Maximets.
> 
> > What do you think?
> >
> > Regards,
> > Ophir
> >
> >> -Original Message-
> >> From: Ilya Maximets 
> >> Sent: Friday, November 8, 2019 5:57 PM
> >> To: Ophir Munk ; Ilya Maximets
> >> ; ovs-dev@openvswitch.org
> >> Cc: Ameer Mahagneh ; Roni Bar Yanai
> >> ; Eveline Raine 
> >> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
> >>
> >> On 06.11.2019 18:11, Ophir Munk wrote:
> >>> Hi Ilya,
> >>> A few months ago we discussed the missing functionality of vports
> >> offloading under user space bridges.
> >>> Commit [1] was added to explicitly avoid installing userspace vport
> >>> flows (to
> >> avoid confusion with the vport kernel).
> >>> When reverting commit [1] - we are left with this missing functionality.
> >>> Applying your suggested patch netdev_vport_has_system_port() API)
> >> does not seem to work.
> >>> It always fails when it tries to look for the interface name (say
> >>> "vxlan10") in
> >> the system list where vxlan interfaces are renamed (say
> "vxlan_sys_4789").
> >>
> >> Hi Ophir,
> >>
> >> No-one ever tried to run this code, so I'm not surprised.
> >> I could take a look at this issue on next week.
> >>
> >> Best regards, Ilya Maximets.
> >>
> >>>
> >>> You wrote:
>  On master, after applying dynamic flow API patch-set, we'll be able
>  to use patch that I suggested in previous mail to properly handle
>  this situation and enable vport offloading.
> >>>
> >>> It would be appreciated if we can resume the efforts in fixing this
> >>> missing
> >> functionality.
> >>> Please advise on the next steps.
> >>>
> >>> [1]
> >>> Commit 0da667e345 ("dpif-netdev: Forbid vport offloading attempts.")
> >>>
> 
>  -Original Message-
>  From: Ilya Maximets 
>  Sent: Monday, May 13, 2019 3:33 PM
>  To: Ophir Munk ; ovs-
> d...@openvswitch.org
>  Cc: Ian Stokes ; Flavio Leitner
>  ; Kevin Traynor ; Roni Bar
>  Yanai ; Finn Christensen ;
> >> Ben
>  Pfaff ; Simon Horman
> ;
> >> Olga
>  Shern ; Asaf Penso ; Oz
>  Shlomo ; Majd Dibbiny 
>  Subject: Re: [PATCH] netdev: Dynamic per-port Flo

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ilya Maximets]

On 10.11.2019 21:17, Ophir Munk wrote:
> Hi Ilya,
> Thanks you for taking care of this.
> Assigning the correct vport flow API is a critical feature for offloading.
> 
> It seems hard to address all different vport configuration scenarios (kernel 
> only, userspace only or both) by just relying on the individual netdev and 
> without knowing the dpif on top.

Yes you're right.  The only module that knows the real mapping
of 'netdev's to 'dpif's is 'ofproto' and we need to get this
information somehow.

> Maybe can move the flow API assignment to the point where dp is actually 
> adding the port netdev and give a hint about the dp.

Since we already have a hint from the upper layers about the
'dpif_class' I'm suggesting to replace it with 'dpif_type'.
In pair with storing this hint inside the netdev we could have
a flexible system without exposing internals to higher layers
or trying to use higher layer from the library code.

Please, take a look at the new version of patches:
  https://mail.openvswitch.org/pipermail/ovs-dev/2019-November/364735.html

> It is also confusing that userspace vport names are "sys_vxlan_4789" and not 
> "usr_vxlan_4789" for example.

Yes, this is a bit confusing, but we, probably, could not just
change it.  At least that we'll have to rewrite a big part of
system tests that relies on the port names in the flow dumps.
We will have to duplicate them for kernel and userspace.
Some external management/monitoring tools could be affected too
because they will have to treat kernel and userspace tunneling
differently.

Best regards, Ilya Maximets.

> What do you think?
> 
> Regards,
> Ophir
> 
>> -Original Message-
>> From: Ilya Maximets 
>> Sent: Friday, November 8, 2019 5:57 PM
>> To: Ophir Munk ; Ilya Maximets
>> ; ovs-dev@openvswitch.org
>> Cc: Ameer Mahagneh ; Roni Bar Yanai
>> ; Eveline Raine 
>> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
>>
>> On 06.11.2019 18:11, Ophir Munk wrote:
>>> Hi Ilya,
>>> A few months ago we discussed the missing functionality of vports
>> offloading under user space bridges.
>>> Commit [1] was added to explicitly avoid installing userspace vport flows 
>>> (to
>> avoid confusion with the vport kernel).
>>> When reverting commit [1] - we are left with this missing functionality.
>>> Applying your suggested patch netdev_vport_has_system_port() API)
>> does not seem to work.
>>> It always fails when it tries to look for the interface name (say 
>>> "vxlan10") in
>> the system list where vxlan interfaces are renamed (say "vxlan_sys_4789").
>>
>> Hi Ophir,
>>
>> No-one ever tried to run this code, so I'm not surprised.
>> I could take a look at this issue on next week.
>>
>> Best regards, Ilya Maximets.
>>
>>>
>>> You wrote:
 On master, after applying dynamic flow API patch-set, we'll be able
 to use patch that I suggested in previous mail to properly handle
 this situation and enable vport offloading.
>>>
>>> It would be appreciated if we can resume the efforts in fixing this missing
>> functionality.
>>> Please advise on the next steps.
>>>
>>> [1]
>>> Commit 0da667e345 ("dpif-netdev: Forbid vport offloading attempts.")
>>>

 -Original Message-
 From: Ilya Maximets 
 Sent: Monday, May 13, 2019 3:33 PM
 To: Ophir Munk ; ovs-dev@openvswitch.org
 Cc: Ian Stokes ; Flavio Leitner
 ; Kevin Traynor ; Roni Bar
 Yanai ; Finn Christensen ;
>> Ben
 Pfaff ; Simon Horman ;
>> Olga
 Shern ; Asaf Penso ; Oz
 Shlomo ; Majd Dibbiny 
 Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.

 On 13.05.2019 14:41, Ophir Munk wrote:
> Hi Ilya,
>
>> -Original Message-
>> From: Ilya Maximets 
>> Sent: Monday, May 13, 2019 12:22 PM
>> To: Ophir Munk ; ovs-
>> d...@openvswitch.org
>> Cc: Ian Stokes ; Flavio Leitner
>> ; Kevin Traynor ; Roni Bar
>> Yanai ; Finn Christensen ;
 Ben
>> Pfaff ; Simon Horman
>> ;
 Olga
>> Shern ; Asaf Penso ; Oz
>> Shlomo 
>> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
>>
>> On 09.05.2019 1:59, Ophir Munk wrote:
>>> Hi Ilya,
>>>
> ...
>>> I am recreating this scenario in my setup.
>>
>> I see. Yes, you're right. And I think that this case could be
>> reproduced on current master without any patches. So, it's a bug
>> that we
 need to fix.
>> Otherwise userspace datapath will try to offload its flows to the
>> unrelated system interfaces. For now we could just forbid
>> offloading to vports in dpif- netdev. I'll prepare the patch. This
>> fix also should be
 backported.
>>
>
> We need a way to enable offloading of vports in dpif-netdev. So if
> you can address It with your new patch - that would be appreciated.

 I'm suggesting disabling because it'll be easy to backport to older
>> branches.
 On master, after applying dynamic flow API patch-set, we'll be able
 to use patch that I sugg

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ophir Munk]

Hi Ilya,
Thanks you for taking care of this.
Assigning the correct vport flow API is a critical feature for offloading.

It seems hard to address all different vport configuration scenarios (kernel 
only, userspace only or both) by just relying on the individual netdev and 
without knowing the dpif on top.
Maybe can move the flow API assignment to the point where dp is actually adding 
the port netdev and give a hint about the dp.
It is also confusing that userspace vport names are "sys_vxlan_4789" and not 
"usr_vxlan_4789" for example.
What do you think?

Regards,
Ophir

> -Original Message-
> From: Ilya Maximets 
> Sent: Friday, November 8, 2019 5:57 PM
> To: Ophir Munk ; Ilya Maximets
> ; ovs-dev@openvswitch.org
> Cc: Ameer Mahagneh ; Roni Bar Yanai
> ; Eveline Raine 
> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
> 
> On 06.11.2019 18:11, Ophir Munk wrote:
> > Hi Ilya,
> > A few months ago we discussed the missing functionality of vports
> offloading under user space bridges.
> > Commit [1] was added to explicitly avoid installing userspace vport flows 
> > (to
> avoid confusion with the vport kernel).
> > When reverting commit [1] - we are left with this missing functionality.
> > Applying your suggested patch netdev_vport_has_system_port() API)
> does not seem to work.
> > It always fails when it tries to look for the interface name (say 
> > "vxlan10") in
> the system list where vxlan interfaces are renamed (say "vxlan_sys_4789").
> 
> Hi Ophir,
> 
> No-one ever tried to run this code, so I'm not surprised.
> I could take a look at this issue on next week.
> 
> Best regards, Ilya Maximets.
> 
> >
> > You wrote:
> >> On master, after applying dynamic flow API patch-set, we'll be able
> >> to use patch that I suggested in previous mail to properly handle
> >> this situation and enable vport offloading.
> >
> > It would be appreciated if we can resume the efforts in fixing this missing
> functionality.
> > Please advise on the next steps.
> >
> > [1]
> > Commit 0da667e345 ("dpif-netdev: Forbid vport offloading attempts.")
> >
> >>
> >> -Original Message-
> >> From: Ilya Maximets 
> >> Sent: Monday, May 13, 2019 3:33 PM
> >> To: Ophir Munk ; ovs-dev@openvswitch.org
> >> Cc: Ian Stokes ; Flavio Leitner
> >> ; Kevin Traynor ; Roni Bar
> >> Yanai ; Finn Christensen ;
> Ben
> >> Pfaff ; Simon Horman ;
> Olga
> >> Shern ; Asaf Penso ; Oz
> >> Shlomo ; Majd Dibbiny 
> >> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
> >>
> >> On 13.05.2019 14:41, Ophir Munk wrote:
> >>> Hi Ilya,
> >>>
>  -Original Message-
>  From: Ilya Maximets 
>  Sent: Monday, May 13, 2019 12:22 PM
>  To: Ophir Munk ; ovs-
> d...@openvswitch.org
>  Cc: Ian Stokes ; Flavio Leitner
>  ; Kevin Traynor ; Roni Bar
>  Yanai ; Finn Christensen ;
> >> Ben
>  Pfaff ; Simon Horman
> ;
> >> Olga
>  Shern ; Asaf Penso ; Oz
>  Shlomo 
>  Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
> 
>  On 09.05.2019 1:59, Ophir Munk wrote:
> > Hi Ilya,
> >
> >>> ...
> > I am recreating this scenario in my setup.
> 
>  I see. Yes, you're right. And I think that this case could be
>  reproduced on current master without any patches. So, it's a bug
>  that we
> >> need to fix.
>  Otherwise userspace datapath will try to offload its flows to the
>  unrelated system interfaces. For now we could just forbid
>  offloading to vports in dpif- netdev. I'll prepare the patch. This
>  fix also should be
> >> backported.
> 
> >>>
> >>> We need a way to enable offloading of vports in dpif-netdev. So if
> >>> you can address It with your new patch - that would be appreciated.
> >>
> >> I'm suggesting disabling because it'll be easy to backport to older
> branches.
> >> On master, after applying dynamic flow API patch-set, we'll be able
> >> to use patch that I suggested in previous mail to properly handle
> >> this situation and enable vport offloading.
> >>
> >
> >>>
> >
> > This patch series is important but one of its main goals is to
> > enable different
>  offloads for different vports under Kernel/userspace.
> > Can you please advise how this goal can be achieved?
> 
>  It looks like there is no way to distinguish system and userspace
>  vports by looking only on netdev. We should check with dpif type.
> >>>
> >>> Agreed. But then looking at your sample patch below you are basing
> >>> your decision on the existence of system port (and not on dpif type).
> >>
> >> Right now 'ifindex' is used for checking, so this is equally racy.
> >>
> >>>   I think it is risky because
> >>> you are dependent on the order of operations: Creation of the system
> >>> port
> >> versus checking for system port existence.
> >>> Which was first (under different scenarios)?
> >>
> >> Actually, port creation and checking will always happen in the same
> >> thread context, so creation and checking will be serialized

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ilya Maximets]

On 06.11.2019 18:11, Ophir Munk wrote:

Hi Ilya,
A few months ago we discussed the missing functionality of vports offloading 
under user space bridges.
Commit [1] was added to explicitly avoid installing userspace vport flows (to 
avoid confusion with the vport kernel).
When reverting commit [1] - we are left with this missing functionality.
Applying your suggested patch netdev_vport_has_system_port() API)  does not 
seem to work.
It always fails when it tries to look for the interface name (say "vxlan10") in the 
system list where vxlan interfaces are renamed (say "vxlan_sys_4789").


Hi Ophir,

No-one ever tried to run this code, so I'm not surprised.
I could take a look at this issue on next week.

Best regards, Ilya Maximets.



You wrote:

On master, after applying dynamic flow API patch-set, we'll be able to use
patch that I suggested in previous mail to properly handle this situation and
enable vport offloading.


It would be appreciated if we can resume the efforts in fixing this missing 
functionality.
Please advise on the next steps.

[1]
Commit 0da667e345 ("dpif-netdev: Forbid vport offloading attempts.")



-Original Message-
From: Ilya Maximets 
Sent: Monday, May 13, 2019 3:33 PM
To: Ophir Munk ; ovs-dev@openvswitch.org
Cc: Ian Stokes ; Flavio Leitner ;
Kevin Traynor ; Roni Bar Yanai
; Finn Christensen ; Ben Pfaff
; Simon Horman ; Olga
Shern ; Asaf Penso ; Oz
Shlomo ; Majd Dibbiny 
Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.

On 13.05.2019 14:41, Ophir Munk wrote:

Hi Ilya,


-Original Message-
From: Ilya Maximets 
Sent: Monday, May 13, 2019 12:22 PM
To: Ophir Munk ; ovs-dev@openvswitch.org
Cc: Ian Stokes ; Flavio Leitner
; Kevin Traynor ; Roni Bar
Yanai ; Finn Christensen ;

Ben

Pfaff ; Simon Horman ;

Olga

Shern ; Asaf Penso ; Oz
Shlomo 
Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.

On 09.05.2019 1:59, Ophir Munk wrote:

Hi Ilya,


...

I am recreating this scenario in my setup.


I see. Yes, you're right. And I think that this case could be
reproduced on current master without any patches. So, it's a bug that we

need to fix.

Otherwise userspace datapath will try to offload its flows to the
unrelated system interfaces. For now we could just forbid offloading
to vports in dpif- netdev. I'll prepare the patch. This fix also should be

backported.




We need a way to enable offloading of vports in dpif-netdev. So if you
can address It with your new patch - that would be appreciated.


I'm suggesting disabling because it'll be easy to backport to older branches.
On master, after applying dynamic flow API patch-set, we'll be able to use
patch that I suggested in previous mail to properly handle this situation and
enable vport offloading.







This patch series is important but one of its main goals is to
enable different

offloads for different vports under Kernel/userspace.

Can you please advise how this goal can be achieved?


It looks like there is no way to distinguish system and userspace
vports by looking only on netdev. We should check with dpif type.


Agreed. But then looking at your sample patch below you are basing
your decision on the existence of system port (and not on dpif type).


Right now 'ifindex' is used for checking, so this is equally racy.


  I think it is risky because
you are dependent on the order of operations: Creation of the system port

versus checking for system port existence.

Which was first (under different scenarios)?


Actually, port creation and checking will always happen in the same thread
context, so creation and checking will be serialized. Kernel should guarantee
the port existence. No races here.



What do you say about the following patch:

diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c int
netdev_ports_insert(struct netdev *netdev, const struct dpif_class
*dpif_class,
  struct dpif_port *dpif_port) @@ -547,8 +555,9 @@
netdev_ports_insert(struct netdev *netdev, const struct dpif_class
*dpif_class,
  netdev_ports_hash(dpif_port->port_no, dpif_class));
  hmap_insert(&ifindex_to_port, &data->ifindex_node, ifindex);
  ovs_mutex_unlock(&netdev_hmap_mutex);

-netdev_init_flow_api(netdev);
+netdev_init_flow_api(netdev, dpif_class->type); /* Pass class
+ type "netdev" or "syste" */

  return 0;
}

It's not a full solution. It is just a hint how to pass the dpif type ("netdev" 
or

"system")  when initializing the flow api.

We can use the dpif type when initializing vport offload.


This is, actually, the first thing I tried. However, this requires exposing the
internals of 'dpif-provider', which is bad from the point of code architecture.

For the below patch code, I missed actual port requesting from the datapath.
Following incremental needed:

---
diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index
82943c102..1c88a91ed 100644
--- a/lib/netdev-vport.c
+++ b/lib/netdev-vport.c
@@ -124,13 +124,28 @@ netdev_vport_class_get_dpif_port(const 

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ophir Munk]

Hi Ilya,
A few months ago we discussed the missing functionality of vports offloading 
under user space bridges.
Commit [1] was added to explicitly avoid installing userspace vport flows (to 
avoid confusion with the vport kernel).
When reverting commit [1] - we are left with this missing functionality.
Applying your suggested patch netdev_vport_has_system_port() API)  does not 
seem to work.
It always fails when it tries to look for the interface name (say "vxlan10") in 
the system list where vxlan interfaces are renamed (say "vxlan_sys_4789").

You wrote: 
> On master, after applying dynamic flow API patch-set, we'll be able to use
> patch that I suggested in previous mail to properly handle this situation and
> enable vport offloading.

It would be appreciated if we can resume the efforts in fixing this missing 
functionality.
Please advise on the next steps.

[1]
Commit 0da667e345 ("dpif-netdev: Forbid vport offloading attempts.")

> 
> -Original Message-
> From: Ilya Maximets 
> Sent: Monday, May 13, 2019 3:33 PM
> To: Ophir Munk ; ovs-dev@openvswitch.org
> Cc: Ian Stokes ; Flavio Leitner ;
> Kevin Traynor ; Roni Bar Yanai
> ; Finn Christensen ; Ben Pfaff
> ; Simon Horman ; Olga
> Shern ; Asaf Penso ; Oz
> Shlomo ; Majd Dibbiny 
> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
> 
> On 13.05.2019 14:41, Ophir Munk wrote:
> > Hi Ilya,
> >
> >> -Original Message-
> >> From: Ilya Maximets 
> >> Sent: Monday, May 13, 2019 12:22 PM
> >> To: Ophir Munk ; ovs-dev@openvswitch.org
> >> Cc: Ian Stokes ; Flavio Leitner
> >> ; Kevin Traynor ; Roni Bar
> >> Yanai ; Finn Christensen ;
> Ben
> >> Pfaff ; Simon Horman ;
> Olga
> >> Shern ; Asaf Penso ; Oz
> >> Shlomo 
> >> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
> >>
> >> On 09.05.2019 1:59, Ophir Munk wrote:
> >>> Hi Ilya,
> >>>
> > ...
> >>> I am recreating this scenario in my setup.
> >>
> >> I see. Yes, you're right. And I think that this case could be
> >> reproduced on current master without any patches. So, it's a bug that we
> need to fix.
> >> Otherwise userspace datapath will try to offload its flows to the
> >> unrelated system interfaces. For now we could just forbid offloading
> >> to vports in dpif- netdev. I'll prepare the patch. This fix also should be
> backported.
> >>
> >
> > We need a way to enable offloading of vports in dpif-netdev. So if you
> > can address It with your new patch - that would be appreciated.
> 
> I'm suggesting disabling because it'll be easy to backport to older branches.
> On master, after applying dynamic flow API patch-set, we'll be able to use
> patch that I suggested in previous mail to properly handle this situation and
> enable vport offloading.
> 

> >
> >>>
> >>> This patch series is important but one of its main goals is to
> >>> enable different
> >> offloads for different vports under Kernel/userspace.
> >>> Can you please advise how this goal can be achieved?
> >>
> >> It looks like there is no way to distinguish system and userspace
> >> vports by looking only on netdev. We should check with dpif type.
> >
> > Agreed. But then looking at your sample patch below you are basing
> > your decision on the existence of system port (and not on dpif type).
> 
> Right now 'ifindex' is used for checking, so this is equally racy.
> 
> >  I think it is risky because
> > you are dependent on the order of operations: Creation of the system port
> versus checking for system port existence.
> > Which was first (under different scenarios)?
> 
> Actually, port creation and checking will always happen in the same thread
> context, so creation and checking will be serialized. Kernel should guarantee
> the port existence. No races here.
> 
> >
> > What do you say about the following patch:
> >
> > diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c int
> > netdev_ports_insert(struct netdev *netdev, const struct dpif_class
> > *dpif_class,
> >  struct dpif_port *dpif_port) @@ -547,8 +555,9 @@
> > netdev_ports_insert(struct netdev *netdev, const struct dpif_class
> > *dpif_class,
> >  netdev_ports_hash(dpif_port->port_no, dpif_class));
> >  hmap_insert(&ifindex_to_port, &data->ifindex_node, ifindex);
> >  ovs_mutex_unlock(&netdev_hmap_mutex);
> >
> > -netdev_init_flow_api(netdev);
> > +netdev_init_flow_api(netdev, dpif_class->type); /* Pass class
> > + type "netdev" or "syste" */
> >
> >  return 0;
> > }
> >
> > It's not a full solution. It is just a hint how to pass the dpif type 
> > ("netdev" or
> "system")  when initializing the flow api.
> > We can use the dpif type when initializing vport offload.
> 
> This is, actually, the first thing I tried. However, this requires exposing 
> the
> internals of 'dpif-provider', which is bad from the point of code 
> architecture.
> 
> For the below patch code, I missed actual port requesting from the datapath.
> Following incremental needed:
> 
> ---
> diff --git a/lib/netdev-vport.c b/lib/net

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ilya Maximets]

On 13.05.2019 15:33, Ophir Munk wrote:
> Hi,
> 
>> -Original Message-
>> From: Ilya Maximets 
>> Sent: Monday, May 13, 2019 12:22 PM
>> To: Ophir Munk ; ovs-dev@openvswitch.org
>> Cc: Ian Stokes ; Flavio Leitner ;
>> Kevin Traynor ; Roni Bar Yanai
>> ; Finn Christensen ; Ben Pfaff
>> ; Simon Horman ; Olga
>> Shern ; Asaf Penso ; Oz
>> Shlomo 
>> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
>>
>>> I am recreating this scenario in my setup.
>>
>> I see. Yes, you're right. And I think that this case could be reproduced on
>> current master without any patches. So, it's a bug that we need to fix.
>> Otherwise userspace datapath will try to offload its flows to the unrelated
>> system interfaces. For now we could just forbid offloading to vports in dpif-
>> netdev. I'll prepare the patch. This fix also should be backported.
>>
> 
> Till now there was no vport dpif-netdev offload. I think the bug you mention 
> was minor (getting a redundant log message like "vxlan_sys_4789 could not be 
> found").
> No harm was caused and vport in dpif-system behaved correctly. 
> Is it considered a broken implementation? Not sure. Maybe we can leave it as 
> is in current OVS (no backporting) and have a fix in this series for the new 
> vport dpif-netdev offload.

The issue will happen if "vxlan_sys_4789" will be eventually found.
For example, this could happen if the kernel datapath runs at the
same time and also has vxlan port. In this case dpif-netdev will
try to install completely unrelated flow to the system interface.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ophir Munk]

Hi,

> -Original Message-
> From: Ilya Maximets 
> Sent: Monday, May 13, 2019 12:22 PM
> To: Ophir Munk ; ovs-dev@openvswitch.org
> Cc: Ian Stokes ; Flavio Leitner ;
> Kevin Traynor ; Roni Bar Yanai
> ; Finn Christensen ; Ben Pfaff
> ; Simon Horman ; Olga
> Shern ; Asaf Penso ; Oz
> Shlomo 
> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
> 
> > I am recreating this scenario in my setup.
> 
> I see. Yes, you're right. And I think that this case could be reproduced on
> current master without any patches. So, it's a bug that we need to fix.
> Otherwise userspace datapath will try to offload its flows to the unrelated
> system interfaces. For now we could just forbid offloading to vports in dpif-
> netdev. I'll prepare the patch. This fix also should be backported.
> 

Till now there was no vport dpif-netdev offload. I think the bug you mention 
was minor (getting a redundant log message like "vxlan_sys_4789 could not be 
found"). 
No harm was caused and vport in dpif-system behaved correctly. 
Is it considered a broken implementation? Not sure. Maybe we can leave it as is 
in current OVS (no backporting) and have a fix in this series for the new vport 
dpif-netdev offload.

Regards,
Ophir
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ilya Maximets]

On 13.05.2019 14:41, Ophir Munk wrote:
> Hi Ilya,
> 
>> -Original Message-
>> From: Ilya Maximets 
>> Sent: Monday, May 13, 2019 12:22 PM
>> To: Ophir Munk ; ovs-dev@openvswitch.org
>> Cc: Ian Stokes ; Flavio Leitner ;
>> Kevin Traynor ; Roni Bar Yanai
>> ; Finn Christensen ; Ben Pfaff
>> ; Simon Horman ; Olga
>> Shern ; Asaf Penso ; Oz
>> Shlomo 
>> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
>>
>> On 09.05.2019 1:59, Ophir Munk wrote:
>>> Hi Ilya,
>>>
> ...
>>> I am recreating this scenario in my setup.
>>
>> I see. Yes, you're right. And I think that this case could be reproduced on
>> current master without any patches. So, it's a bug that we need to fix.
>> Otherwise userspace datapath will try to offload its flows to the unrelated
>> system interfaces. For now we could just forbid offloading to vports in dpif-
>> netdev. I'll prepare the patch. This fix also should be backported.
>>
> 
> We need a way to enable offloading of vports in dpif-netdev. So if you can 
> address
> It with your new patch - that would be appreciated.

I'm suggesting disabling because it'll be easy to backport to older branches.
On master, after applying dynamic flow API patch-set, we'll be able to use patch
that I suggested in previous mail to properly handle this situation and enable
vport offloading.

> 
>>>
>>> This patch series is important but one of its main goals is to enable 
>>> different
>> offloads for different vports under Kernel/userspace.
>>> Can you please advise how this goal can be achieved?
>>
>> It looks like there is no way to distinguish system and userspace vports by
>> looking only on netdev. We should check with dpif type.
> 
> Agreed. But then looking at your sample patch below you are basing your 
> decision 
> on the existence of system port (and not on dpif type).

Right now 'ifindex' is used for checking, so this is equally racy.

>  I think it is risky because
> you are dependent on the order of operations: Creation of the system port 
> versus checking for system port existence.
> Which was first (under different scenarios)?

Actually, port creation and checking will always happen in the same
thread context, so creation and checking will be serialized. Kernel
should guarantee the port existence. No races here.

> 
> What do you say about the following patch:
> 
> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c
> int
> netdev_ports_insert(struct netdev *netdev, const struct dpif_class
> *dpif_class,
>  struct dpif_port *dpif_port)
> @@ -547,8 +555,9 @@ netdev_ports_insert(struct netdev *netdev, const struct
> dpif_class *dpif_class,
>  netdev_ports_hash(dpif_port->port_no, dpif_class));
>  hmap_insert(&ifindex_to_port, &data->ifindex_node, ifindex);
>  ovs_mutex_unlock(&netdev_hmap_mutex);
> 
> -netdev_init_flow_api(netdev);
> +netdev_init_flow_api(netdev, dpif_class->type); /* Pass class type 
> "netdev" or "syste" */
> 
>  return 0;
> }
> 
> It's not a full solution. It is just a hint how to pass the dpif type 
> ("netdev" or "system")  when initializing the flow api.
> We can use the dpif type when initializing vport offload.

This is, actually, the first thing I tried. However, this requires
exposing the internals of 'dpif-provider', which is bad from the
point of code architecture.

For the below patch code, I missed actual port requesting from the
datapath. Following incremental needed:

---
diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
index 82943c102..1c88a91ed 100644
--- a/lib/netdev-vport.c
+++ b/lib/netdev-vport.c
@@ -124,13 +124,28 @@ netdev_vport_class_get_dpif_port(const struct 
netdev_class *class)
 bool
 netdev_vport_has_system_port(const struct netdev *netdev)
 {
-bool found;
+bool found = false;
+const char *name;
+const char *type = "system";
 struct sset names = SSET_INITIALIZER(&names);
 
 ovs_assert(is_vport_class(netdev_get_class(netdev)));
 
-dp_enumerate_names("system", &names);
-found = sset_contains(&names, netdev_get_name(netdev));
+dp_enumerate_names(type, &names);
+SSET_FOR_EACH (name, &names) {
+struct dpif *dpifp = NULL;
+
+if (dpif_open(name, type, &dpifp)) {
+continue;
+}
+
+found = dpif_port_exists(dpifp, netdev_get_name(netdev));
+
+dpif_close(dpifp);
+if (found) {
+break;
+}
+}
 sset_destroy(&names);
 
 return found;
---

> 
>> Something like this (not tested):
>>
>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c index
>> 01e900461..b7b0616ec 100644
>> --- a/lib/netdev-offload-dpdk.c
>> +++ b/lib/netdev-offload-dpdk.c
>> @@ -22,6 +22,7 @@
>>  #include "dpif-netdev.h"
>>  #include "netdev-offload-provider.h"
>>  #include "netdev-provider.h"
>> +#include "netdev-vport.h"
>>  #include "openvswitch/match.h"
>>  #include "openvswitch/vlog.h"
>>  #include "packets.h"
>> @@ -752,6 +753,13 @@ netdev_offload_dpdk_flow_del(struct

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ophir Munk]

Hi Ilya,

> -Original Message-
> From: Ilya Maximets 
> Sent: Monday, May 13, 2019 12:22 PM
> To: Ophir Munk ; ovs-dev@openvswitch.org
> Cc: Ian Stokes ; Flavio Leitner ;
> Kevin Traynor ; Roni Bar Yanai
> ; Finn Christensen ; Ben Pfaff
> ; Simon Horman ; Olga
> Shern ; Asaf Penso ; Oz
> Shlomo 
> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
> 
> On 09.05.2019 1:59, Ophir Munk wrote:
> > Hi Ilya,
> >
...
> > I am recreating this scenario in my setup.
> 
> I see. Yes, you're right. And I think that this case could be reproduced on
> current master without any patches. So, it's a bug that we need to fix.
> Otherwise userspace datapath will try to offload its flows to the unrelated
> system interfaces. For now we could just forbid offloading to vports in dpif-
> netdev. I'll prepare the patch. This fix also should be backported.
> 

We need a way to enable offloading of vports in dpif-netdev. So if you can 
address
It with your new patch - that would be appreciated.

> >
> > This patch series is important but one of its main goals is to enable 
> > different
> offloads for different vports under Kernel/userspace.
> > Can you please advise how this goal can be achieved?
> 
> It looks like there is no way to distinguish system and userspace vports by
> looking only on netdev. We should check with dpif type.

Agreed. But then looking at your sample patch below you are basing your 
decision 
on the existence of system port (and not on dpif type).  I think it is risky 
because
you are dependent on the order of operations: Creation of the system port 
versus checking for system port existence.
Which was first (under different scenarios)?

What do you say about the following patch:

diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c
int
netdev_ports_insert(struct netdev *netdev, const struct dpif_class
*dpif_class,
 struct dpif_port *dpif_port)
@@ -547,8 +555,9 @@ netdev_ports_insert(struct netdev *netdev, const struct
dpif_class *dpif_class,
 netdev_ports_hash(dpif_port->port_no, dpif_class));
 hmap_insert(&ifindex_to_port, &data->ifindex_node, ifindex);
 ovs_mutex_unlock(&netdev_hmap_mutex);

-netdev_init_flow_api(netdev);
+netdev_init_flow_api(netdev, dpif_class->type); /* Pass class type 
"netdev" or "syste" */

 return 0;
}

It's not a full solution. It is just a hint how to pass the dpif type ("netdev" 
or "system")  when initializing the flow api.
We can use the dpif type when initializing vport offload.

> Something like this (not tested):
> 
> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c index
> 01e900461..b7b0616ec 100644
> --- a/lib/netdev-offload-dpdk.c
> +++ b/lib/netdev-offload-dpdk.c
> @@ -22,6 +22,7 @@
>  #include "dpif-netdev.h"
>  #include "netdev-offload-provider.h"
>  #include "netdev-provider.h"
> +#include "netdev-vport.h"
>  #include "openvswitch/match.h"
>  #include "openvswitch/vlog.h"
>  #include "packets.h"
> @@ -752,6 +753,13 @@ netdev_offload_dpdk_flow_del(struct netdev
> *netdev, const ovs_u128 *ufid,  static int
> netdev_offload_dpdk_init_flow_api(struct netdev *netdev)  {
> +if (netdev_vport_is_vport_class(netdev->netdev_class)
> +&& netdev_vport_has_system_port(netdev)) {
> +VLOG_DBG("%s: vport has backing system interface. Skipping.",
> + netdev_get_name(netdev));
> +return EOPNOTSUPP;
> +}
> +
>  return netdev_dpdk_flow_api_supported(netdev) ? 0 : EOPNOTSUPP;  }
> 
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index
> 498aae369..e4d121ed7 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -31,6 +31,7 @@
>  #include "netdev-linux.h"
>  #include "netdev-offload-provider.h"
>  #include "netdev-provider.h"
> +#include "netdev-vport.h"
>  #include "netlink.h"
>  #include "netlink-socket.h"
>  #include "odp-netlink.h"
> @@ -1560,6 +1561,13 @@ netdev_tc_init_flow_api(struct netdev *netdev)
>  int ifindex;
>  int error;
> 
> +if (netdev_vport_is_vport_class(netdev->netdev_class)
> +&& !netdev_vport_has_system_port(netdev)) {
> +VLOG_DBG("%s: vport has no backing system interface. Skipping.",
> + netdev_get_name(netdev));
> +return EOPNOTSUPP;
> +}
> +
>  ifindex = netdev_get_ifindex(netdev);
>  if (ifindex < 0) {
>  VLOG_INFO("init: failed to get ifindex for %s: %s", diff --git 
> a/lib/netdev-
> vport.c b/lib/netdev-vport.c index 92a256af1..82943c102 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -44,6 +44,7 @@
>  #include "simap.h"
>  #include "smap.h"
>  #include "socket-util.h"
> +#include "sset.h"
>  #include "unaligned.h"
>  #include "unixctl.h"
>  #include "openvswitch/vlog.h"
> @@ -120,6 +121,21 @@ netdev_vport_class_get_dpif_port(const struct
> netdev_class *class)
>  return is_vport_class(class) ? vport_class_cast(class)->dpif_port : 
> NULL;  }
> 
> +bool
> +netdev_vport_has_system_port(con

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ilya Maximets]

On 09.05.2019 1:59, Ophir Munk wrote:
> Hi Ilya,
> 
>> -Original Message-
>> From: Ilya Maximets 
>> Sent: Wednesday, April 24, 2019 6:12 PM
>> To: Ophir Munk ; ovs-dev@openvswitch.org
>> Cc: Ian Stokes ; Flavio Leitner ;
>> Kevin Traynor ; Roni Bar Yanai
>> ; Finn Christensen ; Ben Pfaff
>> ; Roi Dayan ; Simon Horman
>> ; Olga Shern ;
>> Asaf Penso ; Oz Shlomo ; Paul
>> Blakey 
>> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
>> ...
 +static int
 +netdev_assign_flow_api(struct netdev *netdev) {
 +struct netdev_registered_flow_api *rfa;
 +
 +CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) {
 +if (!rfa->flow_api->init_flow_api(netdev)) {
 +ovs_refcount_ref(&rfa->refcnt);
 +ovsrcu_set(&netdev->flow_api, rfa->flow_api);
 +VLOG_INFO("%s: Assigned flow API '%s'.",
 +  netdev_get_name(netdev), rfa->flow_api->type);
 +return 0;
 +}
>>>
>>> By this logic the first API which successfully passes init_flow_api() 
>>> becomes
>> the flow_api for this netdev.
>>> Assuming there are 2 vport APIs: 1. system datapath vport API 2. netdev
>> datapath vport API (planned to be added soon).
>>> Assuming both vport APIs successfully pass init_flow_api(), then the first
>> one to be tested will always be chosen and the second one will never be
>> used.
>>> Are we sure that the system datapath vport will not always be chosen with
>> the current netdev_tc_init_flow_api() implementation.
>>> How can we distinguish between a vport created under different
>> datapaths?
>>> This issue must be resolved for this patch.
>>
>> netdev-vport that works in userspace datapath has no backing linux interface
>> --> get_ifindex failure --> no offloading API configured.
> 
> I think the above suggestion will not solve the issue of how we can 
> distinguish between a vport created under different datapaths?
> Relying on get_index will not work in case you have two vports (say both of 
> type "vxlan"): one in kernel and another in userspace. 
> In such a case: the kernel vxlan vport will be added first to linux with 
> device name "vxlan_sys_4789".
> The same name is used by the userspace vport vxlan. Therefore,
> 
> netdev-vport that works in userspace datapath will have the same name of the 
> linux-vport which is already created in linux
> --> get_ifindex will unintentionally succeed --> netdev-vport will falsely 
> select the kernel offloading API (as already described above: "By this logic 
> the first API...").
> 
> I am recreating this scenario in my setup.

I see. Yes, you're right. And I think that this case could be reproduced
on current master without any patches. So, it's a bug that we need to fix.
Otherwise userspace datapath will try to offload its flows to the unrelated
system interfaces. For now we could just forbid offloading to vports in
dpif-netdev. I'll prepare the patch. This fix also should be backported.

> 
> This patch series is important but one of its main goals is to enable 
> different offloads for different vports under Kernel/userspace.
> Can you please advise how this goal can be achieved?

It looks like there is no way to distinguish system and userspace vports
by looking only on netdev. We should check with dpif type.
Something like this (not tested):

diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index 01e900461..b7b0616ec 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -22,6 +22,7 @@
 #include "dpif-netdev.h"
 #include "netdev-offload-provider.h"
 #include "netdev-provider.h"
+#include "netdev-vport.h"
 #include "openvswitch/match.h"
 #include "openvswitch/vlog.h"
 #include "packets.h"
@@ -752,6 +753,13 @@ netdev_offload_dpdk_flow_del(struct netdev *netdev, const 
ovs_u128 *ufid,
 static int
 netdev_offload_dpdk_init_flow_api(struct netdev *netdev)
 {
+if (netdev_vport_is_vport_class(netdev->netdev_class)
+&& netdev_vport_has_system_port(netdev)) {
+VLOG_DBG("%s: vport has backing system interface. Skipping.",
+ netdev_get_name(netdev));
+return EOPNOTSUPP;
+}
+
 return netdev_dpdk_flow_api_supported(netdev) ? 0 : EOPNOTSUPP;
 }
 
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 498aae369..e4d121ed7 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -31,6 +31,7 @@
 #include "netdev-linux.h"
 #include "netdev-offload-provider.h"
 #include "netdev-provider.h"
+#include "netdev-vport.h"
 #include "netlink.h"
 #include "netlink-socket.h"
 #include "odp-netlink.h"
@@ -1560,6 +1561,13 @@ netdev_tc_init_flow_api(struct netdev *netdev)
 int ifindex;
 int error;
 
+if (netdev_vport_is_vport_class(netdev->netdev_class)
+&& !netdev_vport_has_system_port(netdev)) {
+VLOG_DBG("%s: vport has no backing system interface. Skipping.",
+ netdev_get_name(netdev));
+return EOPNOTSUPP;
+   

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ophir Munk]

Hi Ilya,

> -Original Message-
> From: Ilya Maximets 
> Sent: Wednesday, April 24, 2019 6:12 PM
> To: Ophir Munk ; ovs-dev@openvswitch.org
> Cc: Ian Stokes ; Flavio Leitner ;
> Kevin Traynor ; Roni Bar Yanai
> ; Finn Christensen ; Ben Pfaff
> ; Roi Dayan ; Simon Horman
> ; Olga Shern ;
> Asaf Penso ; Oz Shlomo ; Paul
> Blakey 
> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
> ...
> >> +static int
> >> +netdev_assign_flow_api(struct netdev *netdev) {
> >> +struct netdev_registered_flow_api *rfa;
> >> +
> >> +CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) {
> >> +if (!rfa->flow_api->init_flow_api(netdev)) {
> >> +ovs_refcount_ref(&rfa->refcnt);
> >> +ovsrcu_set(&netdev->flow_api, rfa->flow_api);
> >> +VLOG_INFO("%s: Assigned flow API '%s'.",
> >> +  netdev_get_name(netdev), rfa->flow_api->type);
> >> +return 0;
> >> +}
> >
> > By this logic the first API which successfully passes init_flow_api() 
> > becomes
> the flow_api for this netdev.
> > Assuming there are 2 vport APIs: 1. system datapath vport API 2. netdev
> datapath vport API (planned to be added soon).
> > Assuming both vport APIs successfully pass init_flow_api(), then the first
> one to be tested will always be chosen and the second one will never be
> used.
> > Are we sure that the system datapath vport will not always be chosen with
> the current netdev_tc_init_flow_api() implementation.
> > How can we distinguish between a vport created under different
> datapaths?
> > This issue must be resolved for this patch.
> 
> netdev-vport that works in userspace datapath has no backing linux interface
> --> get_ifindex failure --> no offloading API configured.

I think the above suggestion will not solve the issue of how we can distinguish 
between a vport created under different datapaths?
Relying on get_index will not work in case you have two vports (say both of 
type "vxlan"): one in kernel and another in userspace. 
In such a case: the kernel vxlan vport will be added first to linux with device 
name "vxlan_sys_4789".
The same name is used by the userspace vport vxlan. Therefore,

netdev-vport that works in userspace datapath will have the same name of the 
linux-vport which is already created in linux
--> get_ifindex will unintentionally succeed --> netdev-vport will falsely 
select the kernel offloading API (as already described above: "By this logic 
the first API...").

I am recreating this scenario in my setup.

This patch series is important but one of its main goals is to enable different 
offloads for different vports under Kernel/userspace.
Can you please advise how this goal can be achieved?

Regards,
Ophir

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ilya Maximets]

Hi. Thanks for testing!

> Hi,
> Initial tests show that offload is broken as netdev_rte_offloads_flow_put() 
> is not called.
> It is not called because it is not registered.
> When starting debugging it I noticed in function 
> netdev_register_flow_api_provider()
> that we may return an uninitialized error value.
> Because of this in my tests calling netdev_register_flow_api_provider() 
> returned with errors.
>
> Looking at this function:
> Int netdev_register_flow_api_provider(const struct netdev_flow_api 
> *new_flow_api)
> OVS_EXCLUDED(netdev_flow_api_provider_mutex)
> {
> int error; // < should be initialized to 0

Good catch. However this value never used by callers. I'll fix.

> ...
> return error;
> }

> When initializing error to 0 this function returned successfully but 
> netdev_rte_offloads_flow_put() is
> still not called.
> It requires further debugging.

Sorry, It's a stupid bug while bool to int conversion.
Should be fixed by:

diff --git a/lib/netdev-rte-offloads.c b/lib/netdev-rte-offloads.c
index a3f0b82a8..44188dbf5 100644
--- a/lib/netdev-rte-offloads.c
+++ b/lib/netdev-rte-offloads.c
@@ -753,7 +753,7 @@ netdev_rte_offloads_flow_del(struct netdev
*netdev, const ovs_u128 *ufid,
 static int
 netdev_rte_offloads_init_flow_api(struct netdev *netdev)
 {
-return netdev_dpdk_flow_api_supported(netdev);
+return netdev_dpdk_flow_api_supported(netdev) ? 0 : EOPNOTSUPP;
 }
---

I'll update the patch tomorrow.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ophir Munk]

Hi,
Initial tests show that offload is broken as netdev_rte_offloads_flow_put() is 
not called.
It is not called because it is not registered.
When starting debugging it I noticed in function 
netdev_register_flow_api_provider()
that we may return an uninitialized error value. 
Because of this in my tests calling netdev_register_flow_api_provider() 
returned with errors.

Looking at this function:
Int netdev_register_flow_api_provider(const struct netdev_flow_api 
*new_flow_api)
OVS_EXCLUDED(netdev_flow_api_provider_mutex)
{
int error; // < should be initialized to 0
...
return error;
}

When initializing error to 0 this function returned successfully but 
netdev_rte_offloads_flow_put() is
still not called. 
It requires further debugging.

Regards,
Ophir

> -Original Message-
> From: Ophir Munk
> Sent: Tuesday, April 30, 2019 6:02 PM
> To: Ilya Maximets ; ovs-dev@openvswitch.org
> Cc: Ian Stokes ; Flavio Leitner ;
> Kevin Traynor ; Roni Bar Yanai
> ; Finn Christensen ; Ben Pfaff
> ; Roi Dayan ; Simon Horman
> ; Olga Shern ;
> Asaf Penso ; Oz Shlomo ;
> Majd Dibbiny 
> Subject: RE: [PATCH] netdev: Dynamic per-port Flow API.
> 
> > >
> > > 1. This patch changes code related to OVS offload implementation.
> > > OVS
> > offload must be confirmed with this patch before it is accepted.
> >
> > Sure. And I'll appreciate any help with testing as I'm limited with
> > offloading capable hardware.
> >
> 
> Hopefully I will have test results of this patch regarding MARK+RSS offloads
> next week.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ophir Munk]

> >
> > 1. This patch changes code related to OVS offload implementation. OVS
> offload must be confirmed with this patch before it is accepted.
> 
> Sure. And I'll appreciate any help with testing as I'm limited with offloading
> capable hardware.
> 

Hopefully I will have test results of this patch regarding MARK+RSS offloads 
next week.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ilya Maximets]

On 25.04.2019 10:38, Ilya Maximets wrote:
> On 25.04.2019 0:58, Roni Bar Yanai wrote:
>> Hi Ilya,
>>
>> Please see comment/clarification inline.
>> Thanks,
>> Roni
>>
>>> -Original Message-
>>> From: Ilya Maximets 
>>> Sent: Wednesday, April 24, 2019 6:12 PM
>>> To: Ophir Munk ; ovs-dev@openvswitch.org
>>> Cc: Ian Stokes ; Flavio Leitner ; 
>>> Kevin
>>> Traynor ; Roni Bar Yanai ; Finn
>>> Christensen ; Ben Pfaff ; Roi Dayan
>>> ; Simon Horman ; Olga
>>> Shern ; Asaf Penso ; Oz Shlomo
>>> ; Paul Blakey 
>>> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
>>>
>>> On 24.04.2019 17:26, Ophir Munk wrote:
 Hi Ilya,
 Please find comments inline and at the end of this mail.
>>>
>>> Hi. Thanks for review.
>>> Comments inline.
>>>
>>> Best regards, Ilya Maximets.
>>>

 Regards,
 Ophir

> -Original Message-
> From: Ilya Maximets 
> Sent: Tuesday, April 23, 2019 7:19 PM
> To: ovs-dev@openvswitch.org
> Cc: Ian Stokes ; Flavio Leitner ;
> Ophir Munk ; Kevin Traynor
> ; Roni Bar Yanai ; Finn
> Christensen ; Ben Pfaff ; Roi Dayan
> ; Simon Horman ;
> Ilya Maximets 
> Subject: [PATCH] netdev: Dynamic per-port Flow API.
>
> Current issues with Flow API:
>
> * OVS calls offloading functions regardless of successful
>   flow API initialization. (ex. on init_flow_api failure)
> * Static initilaization of Flow API for a netdev_class forbids
>   having different offloading types for different instances
>   of netdev with the same netdev_class. (ex. different vports in
>   'system' and 'netdev' datapaths at the same time)
>
> Solution:
>
> * Move Flow API from the netdev_class to netdev instance.
> * Make Flow API dynamic, i.e. probe the APIs and choose the
>   suitable one.
>

 I suggest distinguishing between initialization and probing.
 The probing you mention is implemented by testing device initialization:
>>> init_flow_api().
 I suggest adding a new probe() API just for the sake of probing. It will 
 check the
>>> netdev struct.
 I suggest  leaving the init_flow_api() API for HW/drivers initialization.
 The HW/driver may fail at initialization, but it does not mean we need to 
 probe
>>> for a new API in that case.
>>>
>>> I also thought about separate 'probe()' api, but it seems that probing
>>> will be equal to 'init()' in most cases. Checking the 'netdev struct'
>>> fro probing is a bad solution as instances of same netdev class could
>>> require different offloading apis. Probing a new api on init failure
>>> could be useful exactly for this case. Anyway we'll be able to improve
>>> the logic of assigning in the future if it will be needed. This is
>>> the single point in code where we have information about all the available
>>> offloading providers.
>>>

> Side effects:
>
> * Flow API providers localized as possible in their modules.
> * Now we have an ability to make runtime checks. For example,
>   we could check if particular device supports features we
>   need, like if dpdk device supports RSS+MARK action.
>
> Signed-off-by: Ilya Maximets 
> ---
>
> Since RFC:
>   * Fixed sparce build.
>   * Some logs turned from INFO to DBG.
>   * Enabled HW offloading on non-Linux systems
> (For testing with dummy provider).
>
>  lib/automake.mk   |   2 +-
>  lib/dpdk.c|   2 +
>  lib/netdev-dpdk.c |  24 +++-
>  lib/netdev-dpdk.h |   3 +
>  lib/netdev-dummy.c|  24 +++-
>  lib/netdev-linux.c|   3 -
>  lib/netdev-linux.h|  10 --
>  lib/netdev-offload-provider.h |  99 +++
>  lib/netdev-provider.h |  67 +--
>  lib/netdev-rte-offloads.c |  40 +-
>  lib/netdev-rte-offloads.h |  41 +--
>  lib/netdev-tc-offloads.c  |  39 --
>  lib/netdev-tc-offloads.h  |  44 ---
>  lib/netdev-vport.c|   6 +-
>  lib/netdev.c  | 221 +++---
>  tests/dpif-netdev.at  |   4 +-
>  tests/ofproto-macros.at   |   1 -
>  17 files changed, 398 insertions(+), 232 deletions(-)  create mode 100644
> lib/netdev-offload-provider.h  delete mode 100644 lib/netdev-tc-offloads.h
>
> diff --git a/lib/automake.mk b/lib/automake.mk index cc5dccf39..b9f3b69e7
> 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -138,6 +138,7 @@ lib_libopenvswitch_la_SOURCES = \
>   lib/netdev-dpdk.h \
>   lib/netdev-dummy.c \
>   lib/netdev-provider.h \
> + lib/netdev-offload-provider.h \

 Please keep alphabetical order (switch between the last 2 lines)

>   lib/netdev-rte-offloads.h \
>   lib/netdev-vport.c \
>   lib/netdev-vport.h \
> @@ -393,7 +394,6 @@ lib_libopenvswi

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ilya Maximets]

On 25.04.2019 0:58, Roni Bar Yanai wrote:
> Hi Ilya,
> 
> Please see comment/clarification inline.
> Thanks,
> Roni
> 
>> -Original Message-
>> From: Ilya Maximets 
>> Sent: Wednesday, April 24, 2019 6:12 PM
>> To: Ophir Munk ; ovs-dev@openvswitch.org
>> Cc: Ian Stokes ; Flavio Leitner ; 
>> Kevin
>> Traynor ; Roni Bar Yanai ; Finn
>> Christensen ; Ben Pfaff ; Roi Dayan
>> ; Simon Horman ; Olga
>> Shern ; Asaf Penso ; Oz Shlomo
>> ; Paul Blakey 
>> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
>>
>> On 24.04.2019 17:26, Ophir Munk wrote:
>>> Hi Ilya,
>>> Please find comments inline and at the end of this mail.
>>
>> Hi. Thanks for review.
>> Comments inline.
>>
>> Best regards, Ilya Maximets.
>>
>>>
>>> Regards,
>>> Ophir
>>>
 -Original Message-
 From: Ilya Maximets 
 Sent: Tuesday, April 23, 2019 7:19 PM
 To: ovs-dev@openvswitch.org
 Cc: Ian Stokes ; Flavio Leitner ;
 Ophir Munk ; Kevin Traynor
 ; Roni Bar Yanai ; Finn
 Christensen ; Ben Pfaff ; Roi Dayan
 ; Simon Horman ;
 Ilya Maximets 
 Subject: [PATCH] netdev: Dynamic per-port Flow API.

 Current issues with Flow API:

 * OVS calls offloading functions regardless of successful
   flow API initialization. (ex. on init_flow_api failure)
 * Static initilaization of Flow API for a netdev_class forbids
   having different offloading types for different instances
   of netdev with the same netdev_class. (ex. different vports in
   'system' and 'netdev' datapaths at the same time)

 Solution:

 * Move Flow API from the netdev_class to netdev instance.
 * Make Flow API dynamic, i.e. probe the APIs and choose the
   suitable one.

>>>
>>> I suggest distinguishing between initialization and probing.
>>> The probing you mention is implemented by testing device initialization:
>> init_flow_api().
>>> I suggest adding a new probe() API just for the sake of probing. It will 
>>> check the
>> netdev struct.
>>> I suggest  leaving the init_flow_api() API for HW/drivers initialization.
>>> The HW/driver may fail at initialization, but it does not mean we need to 
>>> probe
>> for a new API in that case.
>>
>> I also thought about separate 'probe()' api, but it seems that probing
>> will be equal to 'init()' in most cases. Checking the 'netdev struct'
>> fro probing is a bad solution as instances of same netdev class could
>> require different offloading apis. Probing a new api on init failure
>> could be useful exactly for this case. Anyway we'll be able to improve
>> the logic of assigning in the future if it will be needed. This is
>> the single point in code where we have information about all the available
>> offloading providers.
>>
>>>
 Side effects:

 * Flow API providers localized as possible in their modules.
 * Now we have an ability to make runtime checks. For example,
   we could check if particular device supports features we
   need, like if dpdk device supports RSS+MARK action.

 Signed-off-by: Ilya Maximets 
 ---

 Since RFC:
   * Fixed sparce build.
   * Some logs turned from INFO to DBG.
   * Enabled HW offloading on non-Linux systems
 (For testing with dummy provider).

  lib/automake.mk   |   2 +-
  lib/dpdk.c|   2 +
  lib/netdev-dpdk.c |  24 +++-
  lib/netdev-dpdk.h |   3 +
  lib/netdev-dummy.c|  24 +++-
  lib/netdev-linux.c|   3 -
  lib/netdev-linux.h|  10 --
  lib/netdev-offload-provider.h |  99 +++
  lib/netdev-provider.h |  67 +--
  lib/netdev-rte-offloads.c |  40 +-
  lib/netdev-rte-offloads.h |  41 +--
  lib/netdev-tc-offloads.c  |  39 --
  lib/netdev-tc-offloads.h  |  44 ---
  lib/netdev-vport.c|   6 +-
  lib/netdev.c  | 221 +++---
  tests/dpif-netdev.at  |   4 +-
  tests/ofproto-macros.at   |   1 -
  17 files changed, 398 insertions(+), 232 deletions(-)  create mode 100644
 lib/netdev-offload-provider.h  delete mode 100644 lib/netdev-tc-offloads.h

 diff --git a/lib/automake.mk b/lib/automake.mk index cc5dccf39..b9f3b69e7
 100644
 --- a/lib/automake.mk
 +++ b/lib/automake.mk
 @@ -138,6 +138,7 @@ lib_libopenvswitch_la_SOURCES = \
lib/netdev-dpdk.h \
lib/netdev-dummy.c \
lib/netdev-provider.h \
 +  lib/netdev-offload-provider.h \
>>>
>>> Please keep alphabetical order (switch between the last 2 lines)
>>>
lib/netdev-rte-offloads.h \
lib/netdev-vport.c \
lib/netdev-vport.h \
 @@ -393,7 +394,6 @@ lib_libopenvswitch_la_SOURCES += \
lib/netdev-linux.c \
lib/netdev-linux.h \
lib/netdev-tc-offloads.c \
 -  lib/netdev-tc-offloads.h \
lib/netlink-

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Roni Bar Yanai]

Hi Ilya,

Please see comment/clarification inline.
Thanks,
Roni

>-Original Message-
>From: Ilya Maximets 
>Sent: Wednesday, April 24, 2019 6:12 PM
>To: Ophir Munk ; ovs-dev@openvswitch.org
>Cc: Ian Stokes ; Flavio Leitner ; 
>Kevin
>Traynor ; Roni Bar Yanai ; Finn
>Christensen ; Ben Pfaff ; Roi Dayan
>; Simon Horman ; Olga
>Shern ; Asaf Penso ; Oz Shlomo
>; Paul Blakey 
>Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
>
>On 24.04.2019 17:26, Ophir Munk wrote:
>> Hi Ilya,
>> Please find comments inline and at the end of this mail.
>
>Hi. Thanks for review.
>Comments inline.
>
>Best regards, Ilya Maximets.
>
>>
>> Regards,
>> Ophir
>>
>>> -Original Message-
>>> From: Ilya Maximets 
>>> Sent: Tuesday, April 23, 2019 7:19 PM
>>> To: ovs-dev@openvswitch.org
>>> Cc: Ian Stokes ; Flavio Leitner ;
>>> Ophir Munk ; Kevin Traynor
>>> ; Roni Bar Yanai ; Finn
>>> Christensen ; Ben Pfaff ; Roi Dayan
>>> ; Simon Horman ;
>>> Ilya Maximets 
>>> Subject: [PATCH] netdev: Dynamic per-port Flow API.
>>>
>>> Current issues with Flow API:
>>>
>>> * OVS calls offloading functions regardless of successful
>>>   flow API initialization. (ex. on init_flow_api failure)
>>> * Static initilaization of Flow API for a netdev_class forbids
>>>   having different offloading types for different instances
>>>   of netdev with the same netdev_class. (ex. different vports in
>>>   'system' and 'netdev' datapaths at the same time)
>>>
>>> Solution:
>>>
>>> * Move Flow API from the netdev_class to netdev instance.
>>> * Make Flow API dynamic, i.e. probe the APIs and choose the
>>>   suitable one.
>>>
>>
>> I suggest distinguishing between initialization and probing.
>> The probing you mention is implemented by testing device initialization:
>init_flow_api().
>> I suggest adding a new probe() API just for the sake of probing. It will 
>> check the
>netdev struct.
>> I suggest  leaving the init_flow_api() API for HW/drivers initialization.
>> The HW/driver may fail at initialization, but it does not mean we need to 
>> probe
>for a new API in that case.
>
>I also thought about separate 'probe()' api, but it seems that probing
>will be equal to 'init()' in most cases. Checking the 'netdev struct'
>fro probing is a bad solution as instances of same netdev class could
>require different offloading apis. Probing a new api on init failure
>could be useful exactly for this case. Anyway we'll be able to improve
>the logic of assigning in the future if it will be needed. This is
>the single point in code where we have information about all the available
>offloading providers.
>
>>
>>> Side effects:
>>>
>>> * Flow API providers localized as possible in their modules.
>>> * Now we have an ability to make runtime checks. For example,
>>>   we could check if particular device supports features we
>>>   need, like if dpdk device supports RSS+MARK action.
>>>
>>> Signed-off-by: Ilya Maximets 
>>> ---
>>>
>>> Since RFC:
>>>   * Fixed sparce build.
>>>   * Some logs turned from INFO to DBG.
>>>   * Enabled HW offloading on non-Linux systems
>>> (For testing with dummy provider).
>>>
>>>  lib/automake.mk   |   2 +-
>>>  lib/dpdk.c|   2 +
>>>  lib/netdev-dpdk.c |  24 +++-
>>>  lib/netdev-dpdk.h |   3 +
>>>  lib/netdev-dummy.c|  24 +++-
>>>  lib/netdev-linux.c|   3 -
>>>  lib/netdev-linux.h|  10 --
>>>  lib/netdev-offload-provider.h |  99 +++
>>>  lib/netdev-provider.h |  67 +--
>>>  lib/netdev-rte-offloads.c |  40 +-
>>>  lib/netdev-rte-offloads.h |  41 +--
>>>  lib/netdev-tc-offloads.c  |  39 --
>>>  lib/netdev-tc-offloads.h  |  44 ---
>>>  lib/netdev-vport.c|   6 +-
>>>  lib/netdev.c  | 221 +++---
>>>  tests/dpif-netdev.at  |   4 +-
>>>  tests/ofproto-macros.at   |   1 -
>>>  17 files changed, 398 insertions(+), 232 deletions(-)  create mode 100644
>>> lib/netdev-offload-provider.h  delete mode 100644 lib/netdev-tc-offloads.h
>>>
>>> diff --git a/lib/automake.mk b/lib/automake.mk index cc5dccf39..b9f3b69e7
>>> 100644
>>> --- a/lib/automake.mk
>>> +++ b/lib/automake.mk
>>> @@ -138,6 +138,7 @@ lib_libopenvswitch_la_SOURCES = \
>>> lib/netdev-dpdk.h \
>>> lib/netdev-dummy.c \
>>> lib/netdev-provider.h \
>>> +   lib/netdev-offload-provider.h \
>>
>> Please keep alphabetical order (switch between the last 2 lines)
>>
>>> lib/netdev-rte-offloads.h \
>>> lib/netdev-vport.c \
>>> lib/netdev-vport.h \
>>> @@ -393,7 +394,6 @@ lib_libopenvswitch_la_SOURCES += \
>>> lib/netdev-linux.c \
>>> lib/netdev-linux.h \
>>> lib/netdev-tc-offloads.c \
>>> -   lib/netdev-tc-offloads.h \
>>> lib/netlink-conntrack.c \
>>> lib/netlink-conntrack.h \
>>> lib/netlink-notifier.c \
>>> diff --git a/lib/dpdk.c b/lib/dpdk.c
>>> index dc6171546..6c6298635 100644
>>> --- a/lib/dpdk.c
>>> +++ b/lib/dpdk.c

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ilya Maximets]

On 24.04.2019 17:26, Ophir Munk wrote:
> Hi Ilya,
> Please find comments inline and at the end of this mail.

Hi. Thanks for review.
Comments inline.

Best regards, Ilya Maximets.

> 
> Regards,
> Ophir
> 
>> -Original Message-
>> From: Ilya Maximets 
>> Sent: Tuesday, April 23, 2019 7:19 PM
>> To: ovs-dev@openvswitch.org
>> Cc: Ian Stokes ; Flavio Leitner ;
>> Ophir Munk ; Kevin Traynor
>> ; Roni Bar Yanai ; Finn
>> Christensen ; Ben Pfaff ; Roi Dayan
>> ; Simon Horman ;
>> Ilya Maximets 
>> Subject: [PATCH] netdev: Dynamic per-port Flow API.
>>
>> Current issues with Flow API:
>>
>> * OVS calls offloading functions regardless of successful
>>   flow API initialization. (ex. on init_flow_api failure)
>> * Static initilaization of Flow API for a netdev_class forbids
>>   having different offloading types for different instances
>>   of netdev with the same netdev_class. (ex. different vports in
>>   'system' and 'netdev' datapaths at the same time)
>>
>> Solution:
>>
>> * Move Flow API from the netdev_class to netdev instance.
>> * Make Flow API dynamic, i.e. probe the APIs and choose the
>>   suitable one.
>>
> 
> I suggest distinguishing between initialization and probing.
> The probing you mention is implemented by testing device initialization: 
> init_flow_api(). 
> I suggest adding a new probe() API just for the sake of probing. It will 
> check the netdev struct.
> I suggest  leaving the init_flow_api() API for HW/drivers initialization.
> The HW/driver may fail at initialization, but it does not mean we need to 
> probe for a new API in that case.

I also thought about separate 'probe()' api, but it seems that probing
will be equal to 'init()' in most cases. Checking the 'netdev struct'
fro probing is a bad solution as instances of same netdev class could
require different offloading apis. Probing a new api on init failure
could be useful exactly for this case. Anyway we'll be able to improve
the logic of assigning in the future if it will be needed. This is
the single point in code where we have information about all the available
offloading providers.

> 
>> Side effects:
>>
>> * Flow API providers localized as possible in their modules.
>> * Now we have an ability to make runtime checks. For example,
>>   we could check if particular device supports features we
>>   need, like if dpdk device supports RSS+MARK action.
>>
>> Signed-off-by: Ilya Maximets 
>> ---
>>
>> Since RFC:
>>   * Fixed sparce build.
>>   * Some logs turned from INFO to DBG.
>>   * Enabled HW offloading on non-Linux systems
>> (For testing with dummy provider).
>>
>>  lib/automake.mk   |   2 +-
>>  lib/dpdk.c|   2 +
>>  lib/netdev-dpdk.c |  24 +++-
>>  lib/netdev-dpdk.h |   3 +
>>  lib/netdev-dummy.c|  24 +++-
>>  lib/netdev-linux.c|   3 -
>>  lib/netdev-linux.h|  10 --
>>  lib/netdev-offload-provider.h |  99 +++
>>  lib/netdev-provider.h |  67 +--
>>  lib/netdev-rte-offloads.c |  40 +-
>>  lib/netdev-rte-offloads.h |  41 +--
>>  lib/netdev-tc-offloads.c  |  39 --
>>  lib/netdev-tc-offloads.h  |  44 ---
>>  lib/netdev-vport.c|   6 +-
>>  lib/netdev.c  | 221 +++---
>>  tests/dpif-netdev.at  |   4 +-
>>  tests/ofproto-macros.at   |   1 -
>>  17 files changed, 398 insertions(+), 232 deletions(-)  create mode 100644
>> lib/netdev-offload-provider.h  delete mode 100644 lib/netdev-tc-offloads.h
>>
>> diff --git a/lib/automake.mk b/lib/automake.mk index cc5dccf39..b9f3b69e7
>> 100644
>> --- a/lib/automake.mk
>> +++ b/lib/automake.mk
>> @@ -138,6 +138,7 @@ lib_libopenvswitch_la_SOURCES = \
>>  lib/netdev-dpdk.h \
>>  lib/netdev-dummy.c \
>>  lib/netdev-provider.h \
>> +lib/netdev-offload-provider.h \
> 
> Please keep alphabetical order (switch between the last 2 lines)
> 
>>  lib/netdev-rte-offloads.h \
>>  lib/netdev-vport.c \
>>  lib/netdev-vport.h \
>> @@ -393,7 +394,6 @@ lib_libopenvswitch_la_SOURCES += \
>>  lib/netdev-linux.c \
>>  lib/netdev-linux.h \
>>  lib/netdev-tc-offloads.c \
>> -lib/netdev-tc-offloads.h \
>>  lib/netlink-conntrack.c \
>>  lib/netlink-conntrack.h \
>>  lib/netlink-notifier.c \
>> diff --git a/lib/dpdk.c b/lib/dpdk.c
>> index dc6171546..6c6298635 100644
>> --- a/lib/dpdk.c
>> +++ b/lib/dpdk.c
>> @@ -34,6 +34,7 @@
>>  #include "dirs.h"
>>  #include "fatal-signal.h"
>>  #include "netdev-dpdk.h"
>> +#include "netdev-rte-offloads.h"
>>  #include "openvswitch/dynamic-string.h"
>>  #include "openvswitch/vlog.h"
>>  #include "ovs-numa.h"
>> @@ -442,6 +443,7 @@ dpdk_init__(const struct smap *ovs_other_config)
>>
>>  /* Finally, register the dpdk classes */
>>  netdev_dpdk_register();
>> +netdev_dpdk_flow_api_register();
>>  return true;
>>  }
>>
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
>> 47153dc6

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ophir Munk]

Hi Ilya,
Please find comments inline and at the end of this mail.

Regards,
Ophir

> -Original Message-
> From: Ilya Maximets 
> Sent: Tuesday, April 23, 2019 7:19 PM
> To: ovs-dev@openvswitch.org
> Cc: Ian Stokes ; Flavio Leitner ;
> Ophir Munk ; Kevin Traynor
> ; Roni Bar Yanai ; Finn
> Christensen ; Ben Pfaff ; Roi Dayan
> ; Simon Horman ;
> Ilya Maximets 
> Subject: [PATCH] netdev: Dynamic per-port Flow API.
> 
> Current issues with Flow API:
> 
> * OVS calls offloading functions regardless of successful
>   flow API initialization. (ex. on init_flow_api failure)
> * Static initilaization of Flow API for a netdev_class forbids
>   having different offloading types for different instances
>   of netdev with the same netdev_class. (ex. different vports in
>   'system' and 'netdev' datapaths at the same time)
> 
> Solution:
> 
> * Move Flow API from the netdev_class to netdev instance.
> * Make Flow API dynamic, i.e. probe the APIs and choose the
>   suitable one.
> 

I suggest distinguishing between initialization and probing.
The probing you mention is implemented by testing device initialization: 
init_flow_api(). 
I suggest adding a new probe() API just for the sake of probing. It will check 
the netdev struct. 
I suggest  leaving the init_flow_api() API for HW/drivers initialization.
The HW/driver may fail at initialization, but it does not mean we need to probe 
for a new API in that case.

> Side effects:
> 
> * Flow API providers localized as possible in their modules.
> * Now we have an ability to make runtime checks. For example,
>   we could check if particular device supports features we
>   need, like if dpdk device supports RSS+MARK action.
> 
> Signed-off-by: Ilya Maximets 
> ---
> 
> Since RFC:
>   * Fixed sparce build.
>   * Some logs turned from INFO to DBG.
>   * Enabled HW offloading on non-Linux systems
> (For testing with dummy provider).
> 
>  lib/automake.mk   |   2 +-
>  lib/dpdk.c|   2 +
>  lib/netdev-dpdk.c |  24 +++-
>  lib/netdev-dpdk.h |   3 +
>  lib/netdev-dummy.c|  24 +++-
>  lib/netdev-linux.c|   3 -
>  lib/netdev-linux.h|  10 --
>  lib/netdev-offload-provider.h |  99 +++
>  lib/netdev-provider.h |  67 +--
>  lib/netdev-rte-offloads.c |  40 +-
>  lib/netdev-rte-offloads.h |  41 +--
>  lib/netdev-tc-offloads.c  |  39 --
>  lib/netdev-tc-offloads.h  |  44 ---
>  lib/netdev-vport.c|   6 +-
>  lib/netdev.c  | 221 +++---
>  tests/dpif-netdev.at  |   4 +-
>  tests/ofproto-macros.at   |   1 -
>  17 files changed, 398 insertions(+), 232 deletions(-)  create mode 100644
> lib/netdev-offload-provider.h  delete mode 100644 lib/netdev-tc-offloads.h
> 
> diff --git a/lib/automake.mk b/lib/automake.mk index cc5dccf39..b9f3b69e7
> 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -138,6 +138,7 @@ lib_libopenvswitch_la_SOURCES = \
>   lib/netdev-dpdk.h \
>   lib/netdev-dummy.c \
>   lib/netdev-provider.h \
> + lib/netdev-offload-provider.h \

Please keep alphabetical order (switch between the last 2 lines)

>   lib/netdev-rte-offloads.h \
>   lib/netdev-vport.c \
>   lib/netdev-vport.h \
> @@ -393,7 +394,6 @@ lib_libopenvswitch_la_SOURCES += \
>   lib/netdev-linux.c \
>   lib/netdev-linux.h \
>   lib/netdev-tc-offloads.c \
> - lib/netdev-tc-offloads.h \
>   lib/netlink-conntrack.c \
>   lib/netlink-conntrack.h \
>   lib/netlink-notifier.c \
> diff --git a/lib/dpdk.c b/lib/dpdk.c
> index dc6171546..6c6298635 100644
> --- a/lib/dpdk.c
> +++ b/lib/dpdk.c
> @@ -34,6 +34,7 @@
>  #include "dirs.h"
>  #include "fatal-signal.h"
>  #include "netdev-dpdk.h"
> +#include "netdev-rte-offloads.h"
>  #include "openvswitch/dynamic-string.h"
>  #include "openvswitch/vlog.h"
>  #include "ovs-numa.h"
> @@ -442,6 +443,7 @@ dpdk_init__(const struct smap *ovs_other_config)
> 
>  /* Finally, register the dpdk classes */
>  netdev_dpdk_register();
> +netdev_dpdk_flow_api_register();
>  return true;
>  }
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> 47153dc60..c06c9ef81 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -4204,6 +4204,27 @@ unlock:
>  return err;
>  }
> 
> +bool
> +netdev_dpdk_flow_api_supported(struct netdev *netdev) {
> +struct netdev_dpdk *dev;
> +bool ret = false;
> +
> +if (!is_dpdk_class(netdev->netdev_class)) {
> +goto out;
> +}
> +
> +dev = netdev_dpdk_cast(netdev);
> +ovs_mutex_lock(&dev->mutex);
> +if (dev->type == DPDK_DEV_ETH) {
> +/* TODO: Check if we able to offload some minimal flow. */
> +ret = true;
> +}
> +ovs_mutex_unlock(&dev->mutex);
> +out:
> +return ret;
> +}
> +
>  int
>  netdev_dpdk_rte_flow_destroy(struct netdev *netdev,
>   struct rte_flo

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ilya Maximets]

On 24.04.2019 2:43, Ben Pfaff wrote:
> This caused a "sparse" warning for me until I found and fixed a bug in
> our "sparse" support:
> https://mail.openvswitch.org/pipermail/ovs-dev/2019-April/358455.html
> 

Thanks for the report. Your patch looks good.
I tried to figure out why Travis didn't catch this and found that
recent "sparse" is broken for us. i.e. it doesn't check any OVS
source files because of '-MD' option in build commands.
I prepared the workaround:
  https://mail.openvswitch.org/pipermail/ovs-dev/2019-April/358463.html

And also I reported issue to "sparse" devs. Hope, issue will be fixed there.

With both our "sparse" fixes, this patch builds fine with most recent
"sparse" version.

Best regards, Ilya Maximets.


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ben Pfaff]

This caused a "sparse" warning for me until I found and fixed a bug in
our "sparse" support:
https://mail.openvswitch.org/pipermail/ovs-dev/2019-April/358455.html

I haven't fully reviewed this change.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

[Ilya Maximets]

Current issues with Flow API:

* OVS calls offloading functions regardless of successful
  flow API initialization. (ex. on init_flow_api failure)
* Static initilaization of Flow API for a netdev_class forbids
  having different offloading types for different instances
  of netdev with the same netdev_class. (ex. different vports in
  'system' and 'netdev' datapaths at the same time)

Solution:

* Move Flow API from the netdev_class to netdev instance.
* Make Flow API dynamic, i.e. probe the APIs and choose the
  suitable one.

Side effects:

* Flow API providers localized as possible in their modules.
* Now we have an ability to make runtime checks. For example,
  we could check if particular device supports features we
  need, like if dpdk device supports RSS+MARK action.

Signed-off-by: Ilya Maximets 
---

Since RFC:
  * Fixed sparce build.
  * Some logs turned from INFO to DBG.
  * Enabled HW offloading on non-Linux systems
(For testing with dummy provider).

 lib/automake.mk   |   2 +-
 lib/dpdk.c|   2 +
 lib/netdev-dpdk.c |  24 +++-
 lib/netdev-dpdk.h |   3 +
 lib/netdev-dummy.c|  24 +++-
 lib/netdev-linux.c|   3 -
 lib/netdev-linux.h|  10 --
 lib/netdev-offload-provider.h |  99 +++
 lib/netdev-provider.h |  67 +--
 lib/netdev-rte-offloads.c |  40 +-
 lib/netdev-rte-offloads.h |  41 +--
 lib/netdev-tc-offloads.c  |  39 --
 lib/netdev-tc-offloads.h  |  44 ---
 lib/netdev-vport.c|   6 +-
 lib/netdev.c  | 221 +++---
 tests/dpif-netdev.at  |   4 +-
 tests/ofproto-macros.at   |   1 -
 17 files changed, 398 insertions(+), 232 deletions(-)
 create mode 100644 lib/netdev-offload-provider.h
 delete mode 100644 lib/netdev-tc-offloads.h

diff --git a/lib/automake.mk b/lib/automake.mk
index cc5dccf39..b9f3b69e7 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -138,6 +138,7 @@ lib_libopenvswitch_la_SOURCES = \
lib/netdev-dpdk.h \
lib/netdev-dummy.c \
lib/netdev-provider.h \
+   lib/netdev-offload-provider.h \
lib/netdev-rte-offloads.h \
lib/netdev-vport.c \
lib/netdev-vport.h \
@@ -393,7 +394,6 @@ lib_libopenvswitch_la_SOURCES += \
lib/netdev-linux.c \
lib/netdev-linux.h \
lib/netdev-tc-offloads.c \
-   lib/netdev-tc-offloads.h \
lib/netlink-conntrack.c \
lib/netlink-conntrack.h \
lib/netlink-notifier.c \
diff --git a/lib/dpdk.c b/lib/dpdk.c
index dc6171546..6c6298635 100644
--- a/lib/dpdk.c
+++ b/lib/dpdk.c
@@ -34,6 +34,7 @@
 #include "dirs.h"
 #include "fatal-signal.h"
 #include "netdev-dpdk.h"
+#include "netdev-rte-offloads.h"
 #include "openvswitch/dynamic-string.h"
 #include "openvswitch/vlog.h"
 #include "ovs-numa.h"
@@ -442,6 +443,7 @@ dpdk_init__(const struct smap *ovs_other_config)
 
 /* Finally, register the dpdk classes */
 netdev_dpdk_register();
+netdev_dpdk_flow_api_register();
 return true;
 }
 
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 47153dc60..c06c9ef81 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -4204,6 +4204,27 @@ unlock:
 return err;
 }
 
+bool
+netdev_dpdk_flow_api_supported(struct netdev *netdev)
+{
+struct netdev_dpdk *dev;
+bool ret = false;
+
+if (!is_dpdk_class(netdev->netdev_class)) {
+goto out;
+}
+
+dev = netdev_dpdk_cast(netdev);
+ovs_mutex_lock(&dev->mutex);
+if (dev->type == DPDK_DEV_ETH) {
+/* TODO: Check if we able to offload some minimal flow. */
+ret = true;
+}
+ovs_mutex_unlock(&dev->mutex);
+out:
+return ret;
+}
+
 int
 netdev_dpdk_rte_flow_destroy(struct netdev *netdev,
  struct rte_flow *rte_flow,
@@ -4268,8 +4289,7 @@ netdev_dpdk_rte_flow_create(struct netdev *netdev,
 .get_features = netdev_dpdk_get_features,   \
 .get_status = netdev_dpdk_get_status,   \
 .reconfigure = netdev_dpdk_reconfigure, \
-.rxq_recv = netdev_dpdk_rxq_recv,   \
-DPDK_FLOW_OFFLOAD_API
+.rxq_recv = netdev_dpdk_rxq_recv
 
 static const struct netdev_class dpdk_class = {
 .type = "dpdk",
diff --git a/lib/netdev-dpdk.h b/lib/netdev-dpdk.h
index 9bbb8d8d6..60631c4f0 100644
--- a/lib/netdev-dpdk.h
+++ b/lib/netdev-dpdk.h
@@ -34,6 +34,9 @@ struct rte_flow_action;
 
 void netdev_dpdk_register(void);
 void free_dpdk_buf(struct dp_packet *);
+
+bool netdev_dpdk_flow_api_supported(struct netdev *);
+
 int
 netdev_dpdk_rte_flow_destroy(struct netdev *netdev,
  struct rte_flow *rte_flow,
diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 3f90ffa09..2e2e0c2ab 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -24,6 +24,7 @@
 #include "dp-packet.h"
 #include "dpif-netdev.h"
 #include "flow.h"
+#include "netdev-offload-provider.