| Message ID | 20191120152826.25074-1-elibr@mellanox.com |
|---|---|
| Headers | show |
| Series | netdev datapath actions offload | expand |
On 20.11.2019 16:28, Eli Britstein wrote: > Currently, netdev datapath offload only accelerates the flow match > sequence by associating a mark per flow. This series introduces the full > offload of netdev datapath flows by having the HW also perform the flow > actions. > > This series adds HW offload for output, drop, set MAC, set IPv4, set > TCP/UDP ports and tunnel push actions using the DPDK rte_flow API. > > Travis passed: https://travis-ci.org/elibritstein/OVS/builds/614511472 > > Eli Britstein (18): > sparse: rte_flow.h: Adapt according to DPDK 18.11.2 > netdev-offload-dpdk: Refactor flow patterns and actions code > netdev-offload-dpdk-flow: Dynamically allocate pattern items > netdev-offload-dpdk: Fix typo > netdev-dpdk: Improve HW offload flow debuggability > netdev-dpdk: Introduce flow_flush method > netdev-offload-dpdk: Introduce flow flush callback > netdev-offload-dpdk: Framework for actions offload > netdev-dpdk: Introduce rte flow query function > netdev-offload-dpdk: Implement flow stats get > dpif-netdev: Populate dpif class field in offload struct > netdev-dpdk: Getter function for dpdk port id API > netdev-offload-dpdk-flow: Support offload of output action > netdev-offload-dpdk-flow: Support offload of drop action > netdev-offload-dpdk-flow: Support offload of set MAC actions > netdev-offload-dpdk-flow: Support offload of set IPv4 actions > netdev-offload-dpdk-flow: Support offload of clone tnl_push/output > actions > netdev-offload-dpdk-flow: Support offload of set TCP/UDP ports actions > > Ophir Munk (2): > netdev: Add netdev function: flow_stats_get() > dpif-netdev: Read hw stats during flow_dump_next() call > > NEWS | 3 + > include/sparse/rte_flow.h | 810 ++++++++++++++++++++++++++++++--- > lib/automake.mk | 4 +- > lib/dpif-netdev.c | 42 +- > lib/netdev-dpdk.c | 76 ++++ > lib/netdev-dpdk.h | 11 + > lib/netdev-offload-dpdk-flow.c | 916 ++++++++++++++++++++++++++++++++++++++ > lib/netdev-offload-dpdk-private.h | 65 +++ > lib/netdev-offload-dpdk.c | 535 +++------------------- > lib/netdev-offload-provider.h | 7 + > lib/netdev-offload.c | 12 + > lib/netdev-offload.h | 3 + > 12 files changed, 1969 insertions(+), 515 deletions(-) > create mode 100644 lib/netdev-offload-dpdk-flow.c > create mode 100644 lib/netdev-offload-dpdk-private.h > Hi. Thanks for working on this! I really roughly read the patch-set and made a few comments for the places that caught my eyes. Few general comments for the patches: * I don't understand why all of this refactoring was done. Maybe it's because I didn't try to apply the series and look at it as a complete change, but for now it's unclear for me, especially because we already refactored this code few times before and you're reverting some of these changes. Also the need to have netdev-offload-dpdk-flow.c is not clear. * Commit messages are needed. Please, add some with a brief explanation on what happens and why. * If it's not hard, since you will re-spin the series anyway, please, add some periods to the ends of a subject lines. * You need to also update the 'Flow Hardware Offload' section in the Documentation/howto/dpdk.rst. * And, please, mark these new features as experimental in NEWS. Best regards, Ilya Maximets.
On 11/22/2019 6:34 PM, Ilya Maximets wrote: > On 20.11.2019 16:28, Eli Britstein wrote: >> Currently, netdev datapath offload only accelerates the flow match >> sequence by associating a mark per flow. This series introduces the full >> offload of netdev datapath flows by having the HW also perform the flow >> actions. >> >> This series adds HW offload for output, drop, set MAC, set IPv4, set >> TCP/UDP ports and tunnel push actions using the DPDK rte_flow API. >> >> Travis passed: https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F614511472&data=02%7C01%7Celibr%40mellanox.com%7C893d54b1da3748fab12408d76f69dbcf%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637100372770275434&sdata=031VCVHeS7HxQB%2F6PvZHZOIWNGMl1BcZqMM0JbDKaa8%3D&reserved=0 >> >> Eli Britstein (18): >> sparse: rte_flow.h: Adapt according to DPDK 18.11.2 >> netdev-offload-dpdk: Refactor flow patterns and actions code >> netdev-offload-dpdk-flow: Dynamically allocate pattern items >> netdev-offload-dpdk: Fix typo >> netdev-dpdk: Improve HW offload flow debuggability >> netdev-dpdk: Introduce flow_flush method >> netdev-offload-dpdk: Introduce flow flush callback >> netdev-offload-dpdk: Framework for actions offload >> netdev-dpdk: Introduce rte flow query function >> netdev-offload-dpdk: Implement flow stats get >> dpif-netdev: Populate dpif class field in offload struct >> netdev-dpdk: Getter function for dpdk port id API >> netdev-offload-dpdk-flow: Support offload of output action >> netdev-offload-dpdk-flow: Support offload of drop action >> netdev-offload-dpdk-flow: Support offload of set MAC actions >> netdev-offload-dpdk-flow: Support offload of set IPv4 actions >> netdev-offload-dpdk-flow: Support offload of clone tnl_push/output >> actions >> netdev-offload-dpdk-flow: Support offload of set TCP/UDP ports actions >> >> Ophir Munk (2): >> netdev: Add netdev function: flow_stats_get() >> dpif-netdev: Read hw stats during flow_dump_next() call >> >> NEWS | 3 + >> include/sparse/rte_flow.h | 810 ++++++++++++++++++++++++++++++--- >> lib/automake.mk | 4 +- >> lib/dpif-netdev.c | 42 +- >> lib/netdev-dpdk.c | 76 ++++ >> lib/netdev-dpdk.h | 11 + >> lib/netdev-offload-dpdk-flow.c | 916 ++++++++++++++++++++++++++++++++++++++ >> lib/netdev-offload-dpdk-private.h | 65 +++ >> lib/netdev-offload-dpdk.c | 535 +++------------------- >> lib/netdev-offload-provider.h | 7 + >> lib/netdev-offload.c | 12 + >> lib/netdev-offload.h | 3 + >> 12 files changed, 1969 insertions(+), 515 deletions(-) >> create mode 100644 lib/netdev-offload-dpdk-flow.c >> create mode 100644 lib/netdev-offload-dpdk-private.h >> > Hi. Thanks for working on this! > > I really roughly read the patch-set and made a few comments for > the places that caught my eyes. > > Few general comments for the patches: > > * I don't understand why all of this refactoring was done. Maybe it's because > I didn't try to apply the series and look at it as a complete change, but > for now it's unclear for me, especially because we already refactored this > code few times before and you're reverting some of these changes. > Also the need to have netdev-offload-dpdk-flow.c is not clear. I wanted to make it more ordered, and split to a new file as we plan to submit more patches in this area for VXLAN offload and later CT. Those will include a lot of code additions and maybe even more files. So, we can either keep it so a single file won't grow to be very large, or we can postpone it for later if it really grows like this. I prefer to keep it as is, unless you object. What have I reverted? > > * Commit messages are needed. Please, add some with a brief explanation > on what happens and why. I'll elaborate a bit more, though there are some commits (last ones) that there is really nothing to elaborate more than the subject. > > * If it's not hard, since you will re-spin the series anyway, please, add > some periods to the ends of a subject lines. Actually, I've seen that there is inconsistency of this style in OVS. Some commits have this period and some don't. For example in kernel, iproute2, dpdk gits, they are consistent *without* it. I think it should be consistent. I don't see the added value of it, and I think maybe we should adopt the dot-less style. What do you think? > > * You need to also update the 'Flow Hardware Offload' section in the > Documentation/howto/dpdk.rst. OK. In v2 > > * And, please, mark these new features as experimental in NEWS. OK. In v2 > > Best regards, Ilya Maximets.
Hello Eli, On Wed, Nov 20, 2019 at 4:36 PM Eli Britstein <elibr@mellanox.com> wrote: > > Currently, netdev datapath offload only accelerates the flow match > sequence by associating a mark per flow. This series introduces the full > offload of netdev datapath flows by having the HW also perform the flow > actions. > > This series adds HW offload for output, drop, set MAC, set IPv4, set > TCP/UDP ports and tunnel push actions using the DPDK rte_flow API. Has this series been tested with newer DPDK? The best would be with 19.11-rc3, since we are close to release now. Are there some blockers? Thanks.
Hi David, On 11/26/2019 12:08 PM, David Marchand wrote: > Hello Eli, > > On Wed, Nov 20, 2019 at 4:36 PM Eli Britstein <elibr@mellanox.com> wrote: >> Currently, netdev datapath offload only accelerates the flow match >> sequence by associating a mark per flow. This series introduces the full >> offload of netdev datapath flows by having the HW also perform the flow >> actions. >> >> This series adds HW offload for output, drop, set MAC, set IPv4, set >> TCP/UDP ports and tunnel push actions using the DPDK rte_flow API. > Has this series been tested with newer DPDK? > The best would be with 19.11-rc3, since we are close to release now. > Are there some blockers? Tested with 19.11-rc3. Works fine. I don't see any blockers (from this series point of view). > > Thanks. > >
On Tue, Nov 26, 2019 at 2:28 PM Eli Britstein <elibr@mellanox.com> wrote: > >> This series adds HW offload for output, drop, set MAC, set IPv4, set > >> TCP/UDP ports and tunnel push actions using the DPDK rte_flow API. > > Has this series been tested with newer DPDK? > > The best would be with 19.11-rc3, since we are close to release now. > > Are there some blockers? > Tested with 19.11-rc3. Works fine. I don't see any blockers (from this > series point of view). Cool! Thanks. -- David Marchand
On Wed, Nov 20, 2019 at 9:06 PM Eli Britstein <elibr@mellanox.com> wrote: > > Currently, netdev datapath offload only accelerates the flow match > sequence by associating a mark per flow. This series introduces the full > offload of netdev datapath flows by having the HW also perform the flow > actions. > > This series adds HW offload for output, drop, set MAC, set IPv4, set > TCP/UDP ports and tunnel push actions using the DPDK rte_flow API. > > Travis passed: https://travis-ci.org/elibritstein/OVS/builds/614511472 > > Eli Britstein (18): > sparse: rte_flow.h: Adapt according to DPDK 18.11.2 > netdev-offload-dpdk: Refactor flow patterns and actions code > netdev-offload-dpdk-flow: Dynamically allocate pattern items > netdev-offload-dpdk: Fix typo > netdev-dpdk: Improve HW offload flow debuggability > netdev-dpdk: Introduce flow_flush method > netdev-offload-dpdk: Introduce flow flush callback > netdev-offload-dpdk: Framework for actions offload > netdev-dpdk: Introduce rte flow query function > netdev-offload-dpdk: Implement flow stats get > dpif-netdev: Populate dpif class field in offload struct > netdev-dpdk: Getter function for dpdk port id API > netdev-offload-dpdk-flow: Support offload of output action > netdev-offload-dpdk-flow: Support offload of drop action > netdev-offload-dpdk-flow: Support offload of set MAC actions > netdev-offload-dpdk-flow: Support offload of set IPv4 actions > netdev-offload-dpdk-flow: Support offload of clone tnl_push/output > actions Hi Eli, Any reason why tnl_pop is not included in this patch set ? Thanks, -Harsha > netdev-offload-dpdk-flow: Support offload of set TCP/UDP ports actions > > Ophir Munk (2): > netdev: Add netdev function: flow_stats_get() > dpif-netdev: Read hw stats during flow_dump_next() call > > NEWS | 3 + > include/sparse/rte_flow.h | 810 ++++++++++++++++++++++++++++++--- > lib/automake.mk | 4 +- > lib/dpif-netdev.c | 42 +- > lib/netdev-dpdk.c | 76 ++++ > lib/netdev-dpdk.h | 11 + > lib/netdev-offload-dpdk-flow.c | 916 ++++++++++++++++++++++++++++++++++++++ > lib/netdev-offload-dpdk-private.h | 65 +++ > lib/netdev-offload-dpdk.c | 535 +++------------------- > lib/netdev-offload-provider.h | 7 + > lib/netdev-offload.c | 12 + > lib/netdev-offload.h | 3 + > 12 files changed, 1969 insertions(+), 515 deletions(-) > create mode 100644 lib/netdev-offload-dpdk-flow.c > create mode 100644 lib/netdev-offload-dpdk-private.h > > -- > 2.14.5 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 12/5/2019 9:55 AM, Sriharsha Basavapatna wrote: > Hi Eli, > > Any reason why tnl_pop is not included in this patch set ? > > Thanks, > -Harsha Currently offloading of vports does not work. Ilya works on that together with Ophir. However, the decap process in OVS-DPDK is much more complex. It consists of 2 flows - tnl_pop on the PF, and tunnel flow on the vport. So, offloading it will be not as straight forward as simple output, drop or encap (clone) in this series. In HW, once we do "VXLAN_DECAP", the outer header is lost. As the outer header is required in the vport flow, we won't be able to do the decap in the tnl_pop flow. Instead it will be in the vport flow. Furthermore, as those are 2 different flows, we must be able to cope with a scenario that only the tnl_pop flow exist and hit, and the vport flow doesn't. In this case, we must be able to handle this HW miss, and proceed the processing in SW. Those are all covered in the next series we work on, and will submit it once the current output series is accepted.
On Thu, Dec 5, 2019 at 2:38 PM Eli Britstein <elibr@mellanox.com> wrote: > > > On 12/5/2019 9:55 AM, Sriharsha Basavapatna wrote: > > Hi Eli, > > > > Any reason why tnl_pop is not included in this patch set ? > > > > Thanks, > > -Harsha > > Currently offloading of vports does not work. Ilya works on that > together with Ophir. > > However, the decap process in OVS-DPDK is much more complex. It consists > of 2 flows - tnl_pop on the PF, and tunnel flow on the vport. > > So, offloading it will be not as straight forward as simple output, drop > or encap (clone) in this series. > > In HW, once we do "VXLAN_DECAP", the outer header is lost. As the outer > header is required in the vport flow, we won't be able to do the decap > in the tnl_pop flow. > > Instead it will be in the vport flow. > > Furthermore, as those are 2 different flows, we must be able to cope > with a scenario that only the tnl_pop flow exist and hit, and the vport > flow doesn't. > > In this case, we must be able to handle this HW miss, and proceed the > processing in SW. > > Those are all covered in the next series we work on, and will submit it > once the current output series is accepted. > Ok, thanks. -Harsha
Currently, netdev datapath offload only accelerates the flow match sequence by associating a mark per flow. This series introduces the full offload of netdev datapath flows by having the HW also perform the flow actions. This series adds HW offload for output, drop, set MAC, set IPv4, set TCP/UDP ports and tunnel push actions using the DPDK rte_flow API. Travis passed: https://travis-ci.org/elibritstein/OVS/builds/614511472 Eli Britstein (18): sparse: rte_flow.h: Adapt according to DPDK 18.11.2 netdev-offload-dpdk: Refactor flow patterns and actions code netdev-offload-dpdk-flow: Dynamically allocate pattern items netdev-offload-dpdk: Fix typo netdev-dpdk: Improve HW offload flow debuggability netdev-dpdk: Introduce flow_flush method netdev-offload-dpdk: Introduce flow flush callback netdev-offload-dpdk: Framework for actions offload netdev-dpdk: Introduce rte flow query function netdev-offload-dpdk: Implement flow stats get dpif-netdev: Populate dpif class field in offload struct netdev-dpdk: Getter function for dpdk port id API netdev-offload-dpdk-flow: Support offload of output action netdev-offload-dpdk-flow: Support offload of drop action netdev-offload-dpdk-flow: Support offload of set MAC actions netdev-offload-dpdk-flow: Support offload of set IPv4 actions netdev-offload-dpdk-flow: Support offload of clone tnl_push/output actions netdev-offload-dpdk-flow: Support offload of set TCP/UDP ports actions Ophir Munk (2): netdev: Add netdev function: flow_stats_get() dpif-netdev: Read hw stats during flow_dump_next() call NEWS | 3 + include/sparse/rte_flow.h | 810 ++++++++++++++++++++++++++++++--- lib/automake.mk | 4 +- lib/dpif-netdev.c | 42 +- lib/netdev-dpdk.c | 76 ++++ lib/netdev-dpdk.h | 11 + lib/netdev-offload-dpdk-flow.c | 916 ++++++++++++++++++++++++++++++++++++++ lib/netdev-offload-dpdk-private.h | 65 +++ lib/netdev-offload-dpdk.c | 535 +++------------------- lib/netdev-offload-provider.h | 7 + lib/netdev-offload.c | 12 + lib/netdev-offload.h | 3 + 12 files changed, 1969 insertions(+), 515 deletions(-) create mode 100644 lib/netdev-offload-dpdk-flow.c create mode 100644 lib/netdev-offload-dpdk-private.h