Hi Konstantin,

Hi Akhil,

quoted

quoted

quoted

quoted

My vote would probably be for option #2 (use one of the reserved

fields

quoted

quoted

for

quoted

quoted

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

quoted

quoted

changes?

quoted

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.

quoted

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.

quoted

}
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

quoted

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

quoted

quoted

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

quoted

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.