[ovs-dev] [PATCH v3 07/10] netdev-offload-tc: Add recirculation, support via tc chains

Paul Blakey paulb at mellanox.com
Sun Dec 15 13:35:36 UTC 2019


On 12/13/2019 2:05 PM, Ilya Maximets wrote:
>> Hi sorry for late reply, didn't get this email.
>>
>> On 03.12.2019 16:16, Ilya Maximets wrote:
>>   > From: Ilya Maximets <i.maximets at samsung.com>
>>   > On 03.12.2019 14:45, Roi Dayan wrote:
>>   > > From: Paul Blakey <paulb at mellanox.com>
>>   > >

[...]

>>
>> We are calling dpif_set_features, not accessing dpif internals.
> Not here, but you're dereferencing dpif pointer later producing the build
> failure:
> lib/netdev-offload-tc.c:1371:64: error: using member 'dpif_class' in incomplete struct dpif
>
>> The thing is, that this sets a feature and not just probes for a feature.
>>
>> It enables a static branch on the kernel side which we might not want to
>> enable any time,
> Does it have any significant performance impact?  Looking at the kernel
> code I don't see any special heavy operations.

I don't see any heavy operations as well, it was just suggested by 
mailing list,

We just lookup the skb extension on vport recv. For normal packets this 
will just incur a simple inline check of skb_ext_find (which just does 
'return skb->active_extensions & (1 << id)'),

which will return false.


>
>> and an offloading a recirc() action was our hook to know that this is
>> wanted, as we talked
>>
>> about in the kernel patch.
> I do not know what you've talked about while upstreaming kernel parts,
> but current patch-set for OVS doesn't work for several reasons:
>
> 1. Obviously, it doesn't build successfully, because netdev-offload
>     implementation now requires access to the internals of 'struct dpif'.
Right I will check that.
>
> 2. You're using ovsthread_once to not enable feature twice and this will
>     break CT offloading if you'll remove datapath and create it back.  You
>     will not be able to enable feature for the newly created datapath.

Thanks, will fix that.

>
> 3. offloading module doesn't depend on datapath and could be theoretically
>     used from the userspace datapath at the same time and if userspace
>     datapath will reach feature enabling code first it will try to set
>     datapath features, will fail and kernel datapath will never try to
>     enable feature because of the same ovsthread_once.
>
> So, taking above issues into account, feature enabling should definitely
> happen on datapath level and might or might not be controlled by the
> ofproto.
>
>> I'm not sure how to do that from ofproto layer.
> We may not involve the ofproto layer.  For example, you may try to enable
> the feature in dpif_netlink_open() and fallback if not supported.  While
> doing that you may remember the status of this feature and then send the
> status to netdev-offload module via 'struct offload_info' or check directly
> by implementing dpif_get_[user_]features().


As the performance impact is very minor,

I'm for enabling/querying this feature at the start (probably via 
dpif_set_features) after a successfully creation in dpif_netlink_open of 
dpif-netlink,

will that be ok with you?

Thanks,

Paul.



More information about the dev mailing list