[ovs-dev] [PATCH v1 0/3] Move offloading-code into a new file

[Roni Bar Yanai]

Ilya, this is what I think.
I think there are two points here. First is the atomicity of offloading flows and the second is concurrency in offloading on dpif.
Regarding the first point, at this point flows are logically atomic, so if for example you are offloading with two threads the lock
Is required only for the offload action itself (as it today). Nothing breaks if you add from one thread and then from another..
Of course, if that the case the netdev-rte-offloads.c should be thread safe, and also the offload itself should keep flows on same
thread so we won't get out of order for same flow (del, add).
Regarding the seconds point, I think that concurrency should be used only if OVS is the bottleneck.  The number of flows is a function
of where the OVS is used. If OVS is used as VNF load balancer, than maybe few rules will do the job. In case of a cloud for example
you might have hundreds of thousands of flows, and update rate of thousands per second. In many cases, if the latency can get to seconds, by
the time the flow will be offloaded, the flow will probably already has ended. In this case there is also a risk of memory leak, as rules rate is much higher
than apply rate, and rules will pileup in the queue. I think that depending on the scenario there is a max latency requirement. 
It is up to the driver owner to reach that latency, and it can do the optimization in the driver or firmware.
If we already raised this issues, I think that queue size for offload thread should be metered some how to avoid memory leak, and also applying rules 
too late. 

Saying that, we can expose dpdk_port and add some acquire/release to the api in the future if needed. I think that the netde-rte-offloads.c module
uses netdev-dpdk module, same way as RTE_FLOW uses dpdk_port. RTE_FLOW is a framework on top of DPDK.

BR,
Roni 

> -----Original Message-----
> From: Ilya Maximets <i.maximets at samsung.com>
> Sent: Wednesday, February 20, 2019 7:06 PM
> To: Ophir Munk <ophirmu at mellanox.com>; ovs-dev at openvswitch.org
> Cc: Ian Stokes <ian.stokes at intel.com>; Olga Shern <olgas at mellanox.com>;
> Kevin Traynor <ktraynor at redhat.com>; Asaf Penso <asafp at mellanox.com>;
> Roni Bar Yanai <roniba at mellanox.com>; Shahaf Shuler
> <shahafs at mellanox.com>; Flavio Leitner <fbl at sysclose.org>; Finn
> Christensen <fc at napatech.com>
> Subject: Re: [PATCH v1 0/3] Move offloading-code into a new file
> 
> One general concern to think about:
> 
> This code split introduces few helper functions that resides in netdev-dpdk
> and calls dpdk API by the requests of other modules (netdev-rte-offloads).
> This strategy will not allow us to lock the device for performing several
> operations atomically because all the locks remains in netdev-dpdk.
> While we have a single thread that handles all the offloading in dpif-netdev
> it's not an issue. But I'm wondering, what will happen if we'll decide to
> use offload API concurrently ? What will happen if we'll need to perform
> few operations on the same device atomically.
> 
> I'm not sure if we'll need this or not. Maybe someone has ideas or knows
> such scenarios that will require atomicity ?
> Is there need of changing current single-thread model to multi-thread ?
> 
> In fact that some NICs are not fast enough in updating flow tables
> (like cxgbe that could take up to 10 seconds), having multiple offloading
> threads might be a good idea.
> 
> Best regards, Ilya Maximets.
> 
> 
> On 18.02.2019 19:16, Ophir Munk wrote:
> > Hardware offloading-code is moved to a new file called
> > netdev-rte-offloads.c. The original offloading-code is copied as is
> > from the netdev-dpdk.c file to the new file where future
> > offloading-code should be added as well.
> >
> > This series is essential for offloading-code development for the following
> > reasons:
> > 1. This series does not change the existing OVS code flows/logic on master
> branch.
> > OVS functionality is the same before and after this series.
> > 2. The separation is essential for new offloading-code
> > development without interfering with the rest of OVS development.
> > 3. Vice versa: it is essential that while developing offloading-code -
> > to be able to frequently rebase on top of master branch.
> > 4. The OVS-kernel is practicing the same approach. Please note the file
> lib/netdev-tc-offloads.c.
> >
> > Based on the points mentioned above we kindly ask that the series will be
> > applied on top of the master branch.
> >
> > Ophir Munk (1):
> >   netdev-rte-offloads: Rename netdev_dpdk_* functions
> >
> > Roni Bar Yanai (2):
> >   netdev-dpdk: Expose flow creation/destruction calls
> >   netdev-dpdk: Move offloading-code to a new file
> >
> >  lib/automake.mk           |   4 +-
> >  lib/netdev-dpdk.c         | 764 ++-------------------------------------------
> >  lib/netdev-dpdk.h         |  14 +
> >  lib/netdev-rte-offloads.c | 778
> ++++++++++++++++++++++++++++++++++++++++++++++
> >  lib/netdev-rte-offloads.h |  39 +++
> >  5 files changed, 855 insertions(+), 744 deletions(-)
> >  create mode 100644 lib/netdev-rte-offloads.c
> >  create mode 100644 lib/netdev-rte-offloads.h
> >