On 12/08/2014 10:58 AM, Alexandru Badicioiu wrote:
Some comments inline.
Alex
On 5 December 2014 at 17:45, Taras Kondratiuk
<taras.kondrat...@linaro.org <mailto:taras.kondrat...@linaro.org>> 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?
Sure freeing previously allocated buffer is normal, but *allocating* it
in a first place is *not necessary*. So no need to free anything.
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.
No. Difference is that application have to 'free' it *always*. No need
to determine action per event.
Regarding API balancing. It is the same use-case as ingress packets
processing: application can free them, but they are not allocated by
application.
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.
Right, but IMO the approach you have proposed is not acceptable. It
doesn't make sense to freeze the API if small API changes can optimize
application and implementation sides. Especially at these early ODP
versions.
Unfortunately capability call won't solve the issue. Current crypto API
design was build on assumption that implementation can chose to process
a packet differently depending on its characteristics (size, cypher
algorithm, etc.). Depending on a type of processing chosen a completion
event may or may not be needed.
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.
Preserving an original completion buffer content is a wrong assumption.
Current specification doesn't say anything about it. Robby
initially used this as a workaround to avoid completion event
allocation on application side. That's exactly what this RFC is
addressing.
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
lng-odp@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/lng-odp
