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

[Akhil Goyal <gakhil@marvell.com>]

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.