[ovs-dev] [PATCH v6 0/8] netdev datapath: Partial action offload

Hemal Shah hemal.shah at broadcom.com
Fri Jul 17 17:07:49 UTC 2020


Eli,

I do not think the issue you raised is valid. See my comments below!

Hemal

On Wed, Jul 15, 2020 at 10:20 AM Eli Britstein <elibr at mellanox.com> wrote:

> Following my comment about possibility to double encap
> (
> https://patchwork.ozlabs.org/project/openvswitch/patch/20200629095020.8491-4-sriharsha.basavapatna@broadcom.com/),
>
> with this patch-set there are also scenarios in which double push-vlan
> or double pop-vlan will occur.
>
> For example there is a rule to push-vlan to an already tagged packet:
>
> In testpmd format:
>
> flow create 0 egress pattern eth / vlan vid is 10 / end actions
> of_push_vlan ethertype 0x8100 / of_set_vlan_vid vlan_vid 10 / end
>
[Hemal] For an already VLAN tagged packet, you cannot use ethertype 0x8100.
You need to use Q-in-Q ethertype (0x88a8 or 0x9100 - legacy). So, the flow
created above is invalid.

>
> In the period of time the rule has already been in HW until the SW is
> aware of it, the SW still does the action. The HW rule will also hit and
> we'll get double push.
>
[Hemal]
Let us use a valid flow creation.

flow create 0 egress pattern eth / vlan vid is 10 / end actions
of_push_vlan ethertype 0x88a8 / of_set_vlan_vid vlan_vid 10 / end

In this case, the packet with a single VLAN tag with TPID 0x8100 and vlan
vid 10 is matched and another tag with TPID 0x88a8 and vid 10 is added to
the packet by either SW or HW (after the action has been offloaded). If the
SW has already pushed the second VLAN tag, then this packet will not match
in the HW as the TPID check for the flow will fail. So, the HW rule will
not be hit and we will not get double push.

When the SW skips the action, then the HW will match the rule and
appropriately add the second tag.

In either case, I do not see this as an issue.


> In a similar way, double pop may occur.
>
[Hemal] In a similar fashion, I do not see an issue with double pop if
correct flow is created (i.e. right value of TPID is used in matching the
flow).

>
> In general with egress offloading there should be a check not to offload
> in case the packet after sw actions may hit the rule in HW.
>
[Hemal] We cannot add checks for invalid cases. If a user creates flows for
VLAN tagged packets with incorrect TPID values, then that is a user created
invalid configuration.

>
>
> Furthermore, it would be interesting to see some benchmarks that show
> what is the performance gain of such offloads.
>
[Hemal] We would like to have this patch being merged and afterwards we can
provide some performance numbers. Otherwise, it becomes a chicken and egg
problem.

>
>
> On 7/12/2020 10:26 PM, Sriharsha Basavapatna wrote:
> > Hi,
> >
> > This patchset extends the "Partial HW acceleration" mode to offload a
> > part of the action processing to HW, instead of offloading just lookup
> > (MARK/RSS), for "vhost-user" ports. This is referred to as "Partial
> Action
> > Offload". This mode does not require SRIOV/switchdev configuration. In
> this
> > mode, forwarding (output) action is still performed by OVS-DPDK SW
> datapath.
> >
> > Thanks,
> > -Harsha
> >
> > ******
> >
> > v5-->v6:
> >
> > - Addressed the following comments from Ilya:
> >    - Introduced new flow offload APIs for egress & ingress partial
> offload
> >    - Added netdev-offload-dpdk implementation of the new APIs
> >    - Removed netdev specific info from the datapath layer
> >    - Removed flow_api_supported() checks from the datapath layer
> >    - Added dp class-specific assistance to avoid impact to kernel-dp
> (vlan)
> >    - Updated flow-dump identifiers for partial-action offload
> >
> > v4-->v5:
> >
> > - Rebased to the current ovs-master (includes vxlan-encap full offload)
> > - Added 2 patches to support partial offload of vlan push/pop actions
> >
> > v3-->v4:
> >
> > - Removed mega-ufid mapping code from dpif-netdev (patch #5) and updated
> the
> >    existing mega-ufid mapping code in netdev-offload-dpdk to support
> partial
> >    action offload.
> >
> > v2-->v3:
> >
> > - Added more commit log comments in the refactoring patch (#1).
> > - Removed wrapper function for should_partial_offload_egress().
> > - Removed partial offload check for output action in
> parse_flow_actions().
> > - Changed patch sequence (skip-encap and get-stats before offload patch).
> > - Removed locking code (details in email), added inline comments.
> > - Moved mega-ufid mapping code from skip-encap (#3) to the offload patch
> (#5).
> >
> > v1-->v2:
> >
> > Fixed review comments from Eli:
> > - Revamped action list parsing to reject multiple clone/output actions
> > - Updated comments to reflect support for single clone/output action
> > - Removed place-holder function for ingress partial action offload
> > - Created a separate patch (#2) to query dpdk-vhost netdevs
> > - Set transfer attribute to 0 for partial action offload
> > - Updated data type of 'dp_flow' in 'dp_netdev_execute_aux'
> > - Added a mutex to synchronize between offload and datapath threads
> > - Avoid fall back to mark/rss when egress partial offload fails
> > - Drop count action for partial action offload
> >
> > Other changes:
> > - Avoid duplicate offload requests for the same mega-ufid (from PMD
> threads)
> > - Added a coverage counter to track pkts with tnl-push partial offloaded
> > - Fixed dp_netdev_pmd_remove_flow() to delete partial offloaded flow
> >
> > ******
> >
> > Sriharsha Basavapatna (8):
> >    dpif-netdev: Refactor dp_netdev_flow_offload_put()
> >    netdev-dpdk: provide a function to identify dpdk-vhost netdevs
> >    dpif-netdev: Skip encap action during datapath execution
> >    dpif-netdev: Support flow_get() with partial-action-offload
> >    dpif-netdev: Support partial-action-offload of VXLAN encap flow
> >    dpif-netdev: Support partial offload of PUSH_VLAN action
> >    dpif-netdev: Support partial offload of POP_VLAN action
> >    dpctl: update flow-dump output to reflect partial action offload
> >
> >   lib/dpctl.c                   |   5 +-
> >   lib/dpif-netdev.c             | 295 +++++++++++++++++++++++++------
> >   lib/dpif.c                    |   2 +-
> >   lib/netdev-dpdk.c             |   5 +
> >   lib/netdev-dpdk.h             |   1 +
> >   lib/netdev-offload-dpdk.c     | 315 +++++++++++++++++++++++++++++++---
> >   lib/netdev-offload-provider.h |  13 ++
> >   lib/netdev-offload.c          | 102 +++++++++++
> >   lib/netdev-offload.h          |  12 +-
> >   lib/odp-execute.c             |  25 ++-
> >   lib/odp-execute.h             |   5 +-
> >   11 files changed, 693 insertions(+), 87 deletions(-)
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list