Re: [dpdk-dev] [PATCH 2/2] lib/security: add SA lifetime configuration

[Anoob Joseph <anoobj@marvell.com>]

Hi Akhil, Konstantin,

Now that we have an agreement on bitfields (hoping no one else has an objection), I would like to discuss one more topic. It is more related to checksum offload, but it's better that we discuss along with other similar items (like soft expiry).

L3 & L4 checksum can be tristate (CSUM_OK, CSUM_ERROR, CSUM_UNKOWN)

1. Application didn't request. Nothing computed. 
2. Application requested. Checksum verification success.
3. Application requested. Checksum verification failed.
4. Application requested. Checksum could not be computed (PMD limitations etc).

How would we indicate each case?

My proposal would be, let's call the field that we called "warning" as "aux_flags" (auxiliary or secondary information from the operation).

Sequence in the application would be,

if (op.status != SUCCESS) {
    /* handle errors */
}

#define RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECKSUM_COMPUTED (1 << 0)
#define RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECSUM_GOOD (1 << 1)

if (op.aux_flags & RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECKSUM_COMPUTED) {
	if (op.aux_flags & RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECSUM_GOOD)
		mbuf->l4_checksum_good = 1;
	else
		mbuf->l4_checksum_good = 0;
} else {
	if (verify_l4_checksum(mbuf) == SUCCESS) {
		mbuf->l4_checksum_good = 1;
	else
		mbuf->l4_checksum_good = 0;
}

For an application not worried about aux_flags (ex: ipsec-secgw), additional checks are not required. For applications not interested in checksum, a blind check on op.aux_flags would be enough to bail out early. For applications interested in checksum, it can follow above sequence (kinds, for demonstration purpose only). 

Would something like above fine? Or if we want to restrict additional fields for just warnings, (L4_CHECKSUM_ERROR), how would application differentiate between checksum good & checksum not computed? In that case, what should be PMDs treatment of "could not compute" v/s "computed and wrong".

For soft expiry, warning would work fine.

Thanks,
Anoob

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Wednesday, July 28, 2021 6:29 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Anoob Joseph
> <anoobj@marvell.com>; Doherty, Declan <declan.doherty@intel.com>;
> Zhang, Roy Fan <roy.fan.zhang@intel.com>; hemant.agrawal@nxp.com
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Ankur Dwivedi
> <adwivedi@marvell.com>; Tejasree Kondoj <ktejasree@marvell.com>;
> dev@dpdk.org; Archana Muniganti <marchana@marvell.com>
> Subject: RE: [PATCH 2/2] lib/security: add SA lifetime configuration
> 
> Hi Konstantin,
> > Hi Akhil,
> >
> > > > > > My vote would probably be for option #2 (use one of the
> > > > > > reserved
> > fields
> > > > for
> > > > > > it).
> > > > > > That way - existing code wouldn't need to be changed.
> > > > >
> > > > > Adding a single enum or multiple enums is the same thing. Right
> > > > > wrt
> > code
> > > > changes?
> > > > > However, if the check is something like If (status !=
> > > > > RTE_CRYPTO_OP_STATUS_SUCCESS)
> > > > > 	Report appropriate error number App code will need to be
> > > > > updated to take care the warnings in both
> > > > options.
> > > > > It will be something like
> > > > > Option #1
> > > > > If (status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
> > > > > 	If (status < RTE_CRYPTO_OP_STATUS_SUCCESS)
> > > > > 		Report appropriate error number.
> > > > > 	Else
> > > > > 		Report appropriate warning number probably in debug
> > > > prints.
> > > > > }
> > > > > Option #2
> > > > > If (op->status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
> > > > > 	If (op->status == RTE_CRYPTO_OP_STATUS_WARNING) {
> > > > > 		Report appropriate warning based on op->reserved[0]
> > > > > 	} else {
> > > > > 		Report appropriate error number
> > > > > 	}
> > > > > }
> > > > > Here both the options are same wrt performance.
> > > > > But in option #2, driver and app need to fill and decode 2
> > > > > separate
> > > > variables
> > > > > As against 1 variable in option #1
> > > > >
> > > > > In both the options, there will be similar code changes.
> > > > > Do you suspect any other code change?
> > > >
> > > > Hmm, I think there is some sort of contradiction here.
> > > > From Anoob original mail:
> > > > "Both the above will be an IPsec operation completed successfully
> > > > but
> > with
> > > > additional information
> > > > that PMD can pass on to application for indicating status of offloads."
> > > > So my understanding after reading Anoob mail was :
> > > > a) warnings will be set when crypto-op completed successfully, i.e:
> > > >      op->status == RTE_CRYPTO_OP_STATUS_SUCCESS
> > > > b) It is not mandatory for the application to process the warnings.
> > > >     Yes it is a recommended but still an optional.
> > >
> > > If we set op->status = RTE_CRYPTO_OP_STATUS_SUCCESS And then
> check
> > > for warnings with a separate variable there will be an extra check
> > > for every packet even for a success case with no warning.
> >
> > Not really. warning will be within the same 32/64 bits as status.
> > Compilers these days are smart enough to generate code that would read
> > an check them as one value:
> > https://godbolt.org/z/M3f9891zq
> >
> > > This may not be acceptable.
> >
> > I don't think there would be any performance regression, see above.
> > If you are still nervous about possibility of this extra load, I think
> > we can go even one step further and reorder crypto_op fields a bit to
> > have 'status' and 'warning'
> > fields consequent, and put them into one struct to make such
> > optimizations explicit.
> > I.E:
> > union {
> >     uint16_t status_warning;
> >     struct {uint8_t status; uint8_t warning;} };
> 
> Yes this looks a good option and as you checked, the compiled code look
> Same for both the cases, we can explore this option.
> With this  union, it will be a single variable also.
> The major concern I had was performance hit. But if that is not an issue, We
> can work on this one.
> 
> Thanks.
>