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

[Ananyev, Konstantin]

Hi Akhil,

> > > > > There are two options that we considered,
> > > > > 1. Extend the enum, rte_crypto_op_status,  to cover warnings [1]
> > > > > 2. There are reserved fields in rte_cryto_op structure. So we can use
> > bits in
> > > > them to indicate various cases. [2]
> > > > >
> > > > > Both the submitted patches follow approach 1 (following how it's done
> > > > currently), but we can switch to approach 2 if we think there can be
> > > > > more such "warnings" that can occur simultaneously. Can you share
> > your
> > > > thoughts on how we should extend the library to handle such
> > > > > cases?
> > > > >
> > > > > [1] https://doc.dpdk.org/api/rte__crypto_8h.html#afe16508b77c2a8dc5caf74a4e9850171
> > > > > [2] https://doc.dpdk.org/api/rte__crypto_8h_source.html
> > > >
> > > > 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;}
}; 
 
> Now, if  we introduce RTE_CRYPTO_OP_STATUS_WARNING or any other warning,
> Then it would mean a SUCCESS but with a specific warning which application can decide
> to ignore or process. All the enum fields > RTE_CRYPTO_OP_STATUS_SUCCESS Should be
> treated as success.
> Status is a uint8_t which can hold 255 values, we can start the warning from say 128,
> Leaving behind scope for more errors which can be added before
> RTE_CRYPTO_OP_STATUS_SUCCESS
> 
> >
> > Though from your mail it seems visa-versa:
> > Warnings are just some extra error codes (op->status !=
> > RTE_CRYPTO_OP_STATUS_SUCCESS)
> > and obviously each app have to handle them.
> >
> > So could you tell me which approach did you mean?
> > If these 'warnings' are just new error codes and app is required to handle
> > them,
> > then why do we need to introduce 'warnings' at all?
> > Lets treat them as error - add new  RTE_CRYPTO_OP_STATUS_ error codes
> > for them
> > and that's would be it.
> 
> We cannot treat warnings as error codes. These are success cases with some
> caution to inform user that there may be some issue in coming packets, eg soft expiry.
> The patch that Anoob sent and the options that I specified are inline.
> There may be some confusion with the wordings. I hope all your doubts gets clarified
> After this mail.
> 
> >
> > If processing them is optional, then I think we better have a new field for
> > them
> > So app code will look like:
> > if (op->status == RTE_CRYPTO_OP_STATUS_SUCCESS) {
> >     if (op->warning != 0) {
> >         /* handle warning conditions here */
> >     }
> >     /* do normal success processing */
> > }
> >
> > In that case existing apps will be continue to work without any modifications.
> > Yes, they would just ignore these new warnings, but nothing will be broken.
> >
> The existing apps can still work and but they would treat warnings as error for
> the PMDs which can return these warnings. 
> For all other PMDs, it will work as is.
> But the application writer knows the features of the PMD which it is using
> And hence would need to take care of the warnings eventually.
> Eg: it will configure the soft/hard expiry limits while configuring the session.
> Hence it will expect the warning to come.

So, PMD will generate warnings only when particular offloads will be enabled,  
and existing apps wouldn't need to be changed to keep working, right?
That's a good thing.
Though I still don't like the idea to implicitly re-define op->status behaviour,
depending on some offloads enable/disable. 
Warning as separate filed looks much more sane and clear to me.

> Moreover as I said above also, there will be one extra check for each packet even
> for success cases without any warning which may not be desirable.
> As I suggested in both the options, the extra check will be there only in case
> there is error or warning and not on the success case.
> 
> > > > Again these warnings, it probably needs to be a bit-flags, correct?
> > >
> > > We can deal with both bit flags as well as new enums in the status.
> > > I believe both are same and in fact using enum in application is more
> > convenient
> > > for user, instead of decoding bit flags.
> > > However, it is personal choice. People may differ on that.
> >
> > From what I understand from previous mails: same op can have multiple
> > warnings set.
> > Let say both SOFT_LIMIT can be reached and L4 checksum is not correct.
> > That's why I presumed that warnings have to be a bit-flag.
> 
> We can specify enum names to combine the possible combination of warnings.
> Eg: RTE_CRYPTO_OP_STATUS_WAR_SE_L4_CSUM

With just 2 warnings defined it is ok, but imagine in future there would be
let say 5 or 6 different warnings, and nearly all combinations will be possible.
With enum it would become a real pain.