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.dohe...@intel.com>
Signed-off-by: Radu Nicolau <radu.nico...@intel.com>
Signed-off-by: Abhijit Sinha <abhijit.si...@intel.com>
Signed-off-by: Daniel Martin Buckley <daniel.m.buck...@intel.com>
Acked-by: Fan Zhang <roy.fan.zh...@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 :)
