[ovs-dev] [PATCH v5 0/7] netdev datapath: Partial action offload

Ilya Maximets i.maximets at ovn.org
Fri Jul 10 12:11:47 UTC 2020


On 7/9/20 8:47 AM, Sriharsha Basavapatna via dev 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.

Hi.  I like the idea of egress offloading.  It's interesting.
IIUC, HW will perform matching on egress packets and perform actions before
actually sending them. Is that right?

For the implementation I have a few concerns:

1. Why only vhost-user ports?  I mean, egress offloading could be done
   for any ingress port in case ingress port doesn't support full offloading.

   Moreover, you could have classification offloading on ingress and actions
   offloading on egress at the same time.  This might be useful, for example,
   if we have two diferent NICs that both supports offloading, but we have to
   send packets between them.  But, yes, that might be a way for further
   improvement.

   Regarding vhost-user, you're exposing too much of netdev internals to
   datapath layer by checking for a specific netdev type.  This is not
   a good thing to do.  Especially because egress offloading doesn't depend
   on a type of ingress interface.

2. I'm worried about other offload providers like tinux-tc that doesn't know
   anything that happens in dpif-netdev and will not work correctly if
   dpif-netdev will try to use egress offloading on it.
   I see that you're using netdev_dpdk_flow_api_supported() inside the
   dpif-netdev, but that is the violation of netdev-offload abstraction layer. 

   I think, some more generic extension of netdev-offload interface required,
   so all offload providers will be able to use this API.  I mean, egress
   offloading should be possible with TC.  You don't need to add support for
   this, but we should have a generic interface to utilize this support in
   the future.

   At least there should be something more generic like info.offload_direction
   of some enumeration type like
    enum netdev_offload_direction {
        NETDEV_OFFLOAD_INGRESS,
        NETDEV_OFFLOAD_EGRESS,
    };
   And each offload provider should decide by itself if it supports some type
   of offloading or not.
   netdev specific or specific to particular offload provider functions should
   not be used in general outside of their own modules.

3. Some new identifiers for flow dumps to distinguish classification and actions
   partial offloading needed.

4. Ingress partial actions offloading sounds interesting, but current
   implementation forces datapath assistance for such actions.  This will
   significantly impact kernel datapath as all the vlan operations will be
   sent back and forth between kernel and userspace.  And I'm not sure if this
   will even work correctly with current patches.


I didn't make a full review.  These are just quick comments from the architectural
point of view.

Best regards, Ilya Maximets.


More information about the dev mailing list