| Message ID | 1550506563-26782-1-git-send-email-ophirmu@mellanox.com |
|---|---|
| Headers | show |
| Series | Move offloading-code into a new file | expand |
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 >
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@samsung.com> > Sent: Wednesday, February 20, 2019 7:06 PM > To: Ophir Munk <ophirmu@mellanox.com>; ovs-dev@openvswitch.org > Cc: Ian Stokes <ian.stokes@intel.com>; Olga Shern <olgas@mellanox.com>; > Kevin Traynor <ktraynor@redhat.com>; Asaf Penso <asafp@mellanox.com>; > Roni Bar Yanai <roniba@mellanox.com>; Shahaf Shuler > <shahafs@mellanox.com>; Flavio Leitner <fbl@sysclose.org>; Finn > Christensen <fc@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 > >
If we wanted to switch to a multi-threaded offload model today- we could have done it in two steps: 1. Implement a multi-threaded model on current master branch. 2. Do the split. The two steps seem independent. The split is rather technical and there are ways to share/pass locks between two files if needed. I don't think that splitting now can limit any future plans to implement a multi-threaded offload. What do you think? > -----Original Message----- > From: Ilya Maximets <i.maximets@samsung.com> > Sent: Wednesday, February 20, 2019 7:06 PM > To: Ophir Munk <ophirmu@mellanox.com>; ovs-dev@openvswitch.org > Cc: Ian Stokes <ian.stokes@intel.com>; Olga Shern <olgas@mellanox.com>; > Kevin Traynor <ktraynor@redhat.com>; Asaf Penso <asafp@mellanox.com>; > Roni Bar Yanai <roniba@mellanox.com>; Shahaf Shuler > <shahafs@mellanox.com>; Flavio Leitner <fbl@sysclose.org>; Finn > Christensen <fc@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 > >
On Wed, Feb 20, 2019 at 10:25:45PM +0000, Ophir Munk wrote: > If we wanted to switch to a multi-threaded offload model today- we could have done it in two steps: > 1. Implement a multi-threaded model on current master branch. > 2. Do the split. > > The two steps seem independent. The split is rather technical and there are ways to share/pass locks between two files if needed. > I don't think that splitting now can limit any future plans to implement a multi-threaded offload. > > What do you think? Do we really need multi-threaded model? I had a hope that this low transfer rate to update flows in the NIC would be solved by the HW itself. fbl
> -----Original Message----- > From: Flavio Leitner <fbl@sysclose.org> > Sent: Thursday, February 21, 2019 1:40 PM > To: Ophir Munk <ophirmu@mellanox.com> > Cc: Ilya Maximets <i.maximets@samsung.com>; ovs-dev@openvswitch.org; > Ian Stokes <ian.stokes@intel.com>; Olga Shern <olgas@mellanox.com>; > Kevin Traynor <ktraynor@redhat.com>; Asaf Penso <asafp@mellanox.com>; > Roni Bar Yanai <roniba@mellanox.com>; Shahaf Shuler > <shahafs@mellanox.com>; Finn Christensen <fc@napatech.com> > Subject: Re: [PATCH v1 0/3] Move offloading-code into a new file > > On Wed, Feb 20, 2019 at 10:25:45PM +0000, Ophir Munk wrote: > > If we wanted to switch to a multi-threaded offload model today- we could > have done it in two steps: > > 1. Implement a multi-threaded model on current master branch. > > 2. Do the split. > > > > The two steps seem independent. The split is rather technical and there > are ways to share/pass locks between two files if needed. > > I don't think that splitting now can limit any future plans to implement a > multi-threaded offload. > > > > What do you think? > > > Do we really need multi-threaded model? I had a hope that this low > transfer rate to update flows in the NIC would be solved by the HW > itself. > > fbl > You will need multi-thread only if OVS is the bottleneck, this not seems to be the case. I think this is a HW bug/limitation that should be solved there like you said.