Re: [dpdk-dev] [PATCH v9 06/10] ipsec: add transmit segmentation offload support

["Nicolau, Radu" <radu.nicolau@intel.com>]

Hi, I reworked this patch as part of a new patchset, and some comments 
below.

On the comment about the offload capabilities, i.e. what happens if a 
PMD supports SECURITY and TSO but not both in the same time, well, if 
this is the case they should not be set both, and also this can 
theoretically happen with any offloads combinations.

On 10/14/2021 3:42 PM, Ananyev, Konstantin wrote:
>
>>> Add support for transmit segmentation offload to inline crypto processing
>> mode. This offload is not supported by other offload modes, as at a
>> minimum it requires inline crypto for IPsec to be supported on the
>> network interface.
> Thanks for rework.
> It looks much better to me now, but still few more comments.
> Konstantin
>   
>> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
>> Signed-off-by: Abhijit Sinha <abhijit.sinha@intel.com>
>> Signed-off-by: Daniel Martin Buckley <daniel.m.buckley@intel.com>
>> Acked-by: Fan Zhang <roy.fan.zhang@intel.com>
>> ---
>> +	for (i = 0; i != num; i++) {
>> +		nb_segs[i] = esn_outb_nb_segments(mb[i]);
>> +		nb_sqn += nb_segs[i];
>> +		/* setup outer l2 and l3 len for TSO */
>> +		if (nb_segs[i] > 1) {
>> +			if (sa->type & RTE_IPSEC_SATP_MODE_TUNLV4)
>> +				mb[i]->outer_l3_len =
>> +						sizeof(struct rte_ipv4_hdr);
>> +			else
>> +				mb[i]->outer_l3_len =
>> +						sizeof(struct rte_ipv6_hdr);
>> +			mb[i]->outer_l2_len = mb[i]->l2_len;
> I still don't understand your logic beyond setting these fields here.
> How it looks to me:
> It is a tunnel mode, so ipsec lib appends it's tunnel header.
> In normal case (non-TSO) it sets up l2_len and l3_len that are stored  inside sa->tx_offload
> (for non-TSO case we don't care about inner/outer case and have to setup outer fields or
> set TX_PKT_OUTER flags).
> Now for TSO we do need to do that, right?
> So as I understand:
> sa->tx_offload.l2_len will become mb->outer_l2_len
> sa->tx_offload.l3_len will become mb->outer_l3_len
> mb->l2_len should be set to zero
> mb->l3_len, mb->l4_len, mb->tso_segsz should remain the same
> (ipsec lib shouldn't modify them).
> Please correct me, if I missed something here.
> Also note that right now we setup mbuf tx_offload way below
> these lines - at outb_tun_pkt_prepare().
> So probably these changes has to be adjusted after that function call.
I removed this section, I think it's best to leave the upper layer to 
set these fields anyway.
>
> 		}
>> +	}
>>
>> -	n = num;
>> +	n = nb_sqn;
>>   	sqn = esn_outb_update_sqn(sa, &n);
>> -	if (n != num)
>> +	if (n != nb_sqn)
>>   		rte_errno = EOVERFLOW;
>>
>>   	k = 0;
>> -	for (i = 0; i != n; i++) {
>> +	for (i = 0; i != num; i++) {
> As I stated that in previous mail, you can't just assume that n == num always.
> That way you just ignores SQN overflow error you get above.
> The proper way - would be to find for how many full packets you have
> valid SQN value and set 'n' to it.
> I know it is an extra pain for TSO mode, but I don't see any better way here.
I reworked this, I hope I got it right this time :)