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

[Akhil Goyal <gakhil@marvell.com>]

Hi Konstantin,
> > > > 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.
This may not be acceptable.
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.

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