Some comments inline. Alex
On 5 December 2014 at 17:45, Taras Kondratiuk <[email protected]> wrote: > On 12/05/2014 04:16 PM, Alexandru Badicioiu wrote: > >> Hi , >> what this patch wants to accomplish can be done with the actual API this >> way : if the implementation really cannot reuse the input packet as the >> completion event (are there implementations which cannot?) it can return >> an error and the application could allocate a separate completion event: >> There is a little overhead with the first assignment and with the failed >> crypto operation in case of platforms which do not support reusing the >> packet but the API doesn't need to be modified (only adding a new error >> code). >> >> odp_buffer_t compl = odp_packet_to_buffer(op_params.pkt); >> rc = odp_crypto_operation(&op_params, &posted, compl); >> if (odp_unlikely(ODP_CRYPTO_INVALID_COMPL_EVENT == rc)) { >> compl = odp_buffer_alloc(); >> rc = odp_crypto_operation(&op_params, &posted, compl); >> } >> > > This approach really looks like a hack and it has several issues: > 1. Application have to remember to free completion in case it was > allocated. [Alex] This is normal, right? If the application allocated something, shouldn't have the responsibility to free it? I think it is the case of your patch when the application has to remember to free something which it did not allocate. You have proposed completion_event_free API but there is no alloc call which is unbalanced. The call is in fact no different than normal buffer free and for implementations which reuse the packet it will be empty, as it is in the patch. > In asynchronous case completion event may be dequeued > in another thread, so 'need to be freed' information should be > passed somehow along with the completion event. Implementation is in > much better position to handle this case. > [Alex] The operation context is meant to "pass something along with the completion event", in case the application really needs this. However, the code example was only to show that the application has a mean to handle this packet vs new completion event issue. I think the API should be changed only if there's no other acceptable way of doing something. The cleanest approach is probably to introduce a capability call and the application can check if it can pass the input packet as the completion event. > 2. Implicitly passing packet as a completion event may mislead > application developer, because he may assume that he will get it back > in exactly the same state (that's exactly what happened in our IPsec > application). [Alex] The underlying buffer is passed, not the packet as such. Implementation knows where the packet and its metadata are in the packet and if no INVALID_COMPLETION_EVENT error is returned it should be guaranteed that the packet "state" is preserved. As I remember from the design, the completion event is just a buffer where the crypto operation returns the completion information. The only "opaque" thing is how the crypto uses the buffer and how it lays out the output information. > But accordingly to the specification completion event > is an opaque token, so developer should not assume that its state is > preserved. Instead we have accessors functions to get information > from the completion event. >
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
