Hi Ilya,
Thanks for your comments, please see my response inline.
On Fri, Jul 10, 2020 at 5:41 PM Ilya Maximets <i.maxim...@ovn.org> wrote:
>
> 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.
Yes, you get the performance benefits of flow offload to virtual NICs
in a VM without SR-IOV (VFs).
> IIUC, HW will perform matching on egress packets and perform actions before
> actually sending them. Is that right?
Right, HW matches on the rule and performs the subset of actions
specified before transmitting. For example, egress packet from the VM
(from a virtio nic) is processed by OVS and OVS forwards it to the
physical port (PF) and the packet is encapsulated (VXLAN) on its way
out to the wire.
>
> 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.
You are right. But just to reduce the scope of initial implementation
and validation, we focused on the use-case involving vhost-user ports.
Once this design is in place, it should be pretty easy to remove this
restriction to include more non-offload capable ingress ports.
>
> 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.
Yes, good idea. This use case can be supported in the future, with
some slight extensions to the current design. Forwarding between two
physical ports, but since they are from different NICs (i.e, they
don't belong to the same eSwitch), break up the offload flow rule into
2 separate rules, with classification on the ingress-port and
partial-action offloading on the egress-port.
>
> 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.
Sure, I'll remove this netdev type specific check from dpif-netdev.
But the current use case for egress offload that we are considering,
is only when the ingress device is not offload capable. So, I'd like
to retain this condition, but I'll move this into the offload layer
(see below).
>
> 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.
Maybe I'm missing something here, but this cannot happen since
dpif-netdev handles only user (dpdk) datapath flows and thus should
only be talking to netdev-offload-dpdk offload provider and in turn
only to devices of class netdev-dpdk ? But I'll add additional checks
to the new offload APIs to make sure only flows of a given datapath
class are processed.
> 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.
Sure, this will be moved into the offload layer API that will be
introduced (see below).
>
> 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.
I agree; will implement the following new offload APIs:
netdev-offload.h: netdev_partial_offload_egress()
netdev-offload.h: netdev_partial_offload_ingress()
netdev-offload-provider.h: netdev_flow_api::flow_offload_egress_partial()
netdev-offload-provider.h: netdev_flow_api::flow_offload_ingress_partial()
netdev-offload-dpdk.c: netdev_offload_dpdk_egress_partial()
netdev-offload-dpdk.c: netdev_offload_dpdk_ingress_partial()
> 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.
One way to provide this additional info might be to represent the
dp_layer in mixed mode (classification + partial-actions), as
"ovs,dpdk" ? And the corresponding offloaded status as
"partial-action". Does this sound ok ? If and when partial action mode
is supported with TC, dp_layer value would be: "ovs,tc" and its
offloaded status would be: "partial-action".
So we would have these combinations:
Classification (mark, rss) offload: dp: ovs, offloaded: partial
Partial-actions offload: dp: ovs,dpdk offloaded:
partial-action
Full-offload: dp: dpdk offloaded:yes
No-offload: dp: ovs
>
> 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.
Thanks for catching this. I'll add a datapath-class specific
assistance callback and the dpif-netdev implementation of this
callback will handle vlan operations. Thus, it shouldn't impact the
kernel datapath.
>
>
> I didn't make a full review. These are just quick comments from the
> architectural
> point of view.
>
Thanks,
-Harsha
> Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
