Message-ID: <4cbebace-2c97-cdd3-384e-f52492b298b7@gmail.com>
Date:   Mon, 30 Nov 2020 09:35:56 +0200
From:   Boris Pismenny <borispismenny@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        Tariq Toukan <ttoukan.linux@...il.com>
Cc:     Jarod Wilson <jarod@...hat.com>, Tariq Toukan <tariqt@...dia.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Saeed Mahameed <saeedm@...dia.com>,
        Moshe Shemesh <moshe@...dia.com>,
        Maor Gottlieb <maorg@...dia.com>
Subject: Re: [PATCH net-next 0/2] TLS TX HW offload for Bond



On 23/11/2020 20:20, Jakub Kicinski wrote:
> On Sun, 22 Nov 2020 14:48:04 +0200 Tariq Toukan wrote:
>>
>> As I understand it, best if we can even generalize this to apply to all 
>> kinds of traffic: bond driver won't do the xmit itself anymore, it just 
>> picks an egress dev and returns it. The core infrastructure will call 
>> the xmit function for the egress dev.
> I think you went way further than I was intending :) I was only
> considering the control path. Leave the datapath unchanged.
>
> AFAIK you're making 3 changes:
>  - forwarding tls ops
>  - pinning flows
>  - handling features
>
> Pinning of the TLS device to a leg of the bond looks like ~15LoC.
> I think we can live with that.
>
> It's the 150 LoC of forwarding TLS ops and duplicating dev selection
> logic in bond_sk_hash_l34() that I'd rather avoid.
>
> Handling features is probably fine, too, I haven't thought about that
> much.

Sorry for jumping in late, but I'd like to present an argument in favor of the approach in the original patch-set, as it may have been overlooked.

The forwarding of TLS ops approach is very flexible, and it will enable support for per-SKB hashing in the future (high-availability): This will require taking ooo_okay into consideration and offloading the context to more than one NIC. But, I think its doable. Even though this approach requires more lines of code, it is already used by other offloads. For instance, XFRM offload in bond_main.c.


>> I like the idea, it can generalize code structures for all kinds of 
>> upper-devices and sockets, taking them into a common place in core, 
>> which reduces code duplications.
>>
>> If we go only half the way, i.e. keep xmit logic in bond for 
>> non-TLS-offloaded traffic, then we have to let TLS module (and others in 
>> the future) act deferentially for different kinds of devs (upper/lower) 
>> which IMHO reduces generality.
> How so? I was expecting TLS to just do something like:
>
> 	netdev = sk_get_xmit_dev_lowest(sk);
>
> which would recursively call get_xmit_slave(CONST) until it reaches
> a device which doesn't resolve further.
>
> BTW is the flow pinning to bond legs actually a must-do? I don't know
> much about bonding but wouldn't that mean that if the selected leg goes
> down we'd lose connectivity, rather than falling back to SW crypto?

It is definitely not a must, and I think we should remove it in the future, once the use-case presents itself.


>> What if the egress dev is detached form the bond? We must then be 
>> notified somehow.
> Do we notify TLS when routing changes? I think it's a separate topic. 
>
> If we have the code to "un-offload" a flow we could handle clearing
> features better and notify from sk_validate_xmit_skb that the flow
> started hitting unexpected dev, hence it should be re-offloaded.
>
> I don't think we need an explicit invalidation from the particular
> drivers here.

Even though re-offload is not exercised, it is possible:
if packets are not using offload by the old netdev, then remove offload from it, and add offload to the new netdev.
A resync, will likely follow, after which offload continue on the new netdev.

The question is who identifies/decides when to re-offload. One option is that the bond driver will trigger it.

Powered by blists - more mailing lists