| Message ID | 20170410230201.15919-1-dmitry.ereminsolenikov@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
> -----Original Message----- > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of > Dmitry Eremin-Solenikov > Sent: Tuesday, April 11, 2017 2:02 AM > To: lng-odp@lists.linaro.org > Subject: [lng-odp] [PATCH] api: ipsec: change semantics of > odp_ipsec_result function > > - Move packets from the event instead of copying them. This simplifies > event handling/freeing code, which now does not have to track, which > packets were copied from the event and which packets should be freed. Current spec is stateless. If array is too small in the first call, application must call again with larger array. Application must not proceeding to access packets, before large enough array is used and result event is "freed". > > - Do not require to free the event before processing packets. This > allows one to copy packets from the event in small batches and > process them accordingly. This *disallows* implementation to use the packet as the results event. Current spec allows implementation (e.g HW) to enqueue a packet with an IPSEC flag set, and use that as the result event. The conversion function + free can modify the event type from IPSEC result to packet. This is how crypto works today, although the spec is not explicit enough about it. -Petri > > Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org> > --- > include/odp/api/spec/ipsec.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/include/odp/api/spec/ipsec.h b/include/odp/api/spec/ipsec.h > index a0ceb11a..8c7750f7 100644 > --- a/include/odp/api/spec/ipsec.h > +++ b/include/odp/api/spec/ipsec.h > @@ -1278,8 +1278,7 @@ int odp_ipsec_out_inline(const odp_ipsec_op_param_t > *op_param, > * Get IPSEC results from an ODP_EVENT_IPSEC_RESULT event > * > * Copies IPSEC operation results from an event. The event must be of > - * type ODP_EVENT_IPSEC_RESULT. It must be freed before the application > passes > - * any resulting packet handles to other ODP calls. > + * type ODP_EVENT_IPSEC_RESULT. It must be freed via odp_event_free(). > * > * @param[out] result Pointer to operation result for output. Maybe > NULL, if > * application is interested only on the number of > @@ -1288,7 +1287,8 @@ int odp_ipsec_out_inline(const odp_ipsec_op_param_t > *op_param, > * > * @return Number of packets in the event. If this is larger than > * 'result.num_pkt', all packets did not fit into result struct > and > - * application must call the function again with a larger result > struct. > + * application must call the function again. Packets returned > during > + * previous calls will not be returned again in subsequent calls. > * @retval <0 On failure > * > * @see odp_ipsec_in_enq(), odp_ipsec_out_enq() > -- > 2.11.0
On 12.04.2017 12:44, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of >> Dmitry Eremin-Solenikov >> Sent: Tuesday, April 11, 2017 2:02 AM >> To: lng-odp@lists.linaro.org >> Subject: [lng-odp] [PATCH] api: ipsec: change semantics of >> odp_ipsec_result function >> >> - Move packets from the event instead of copying them. This simplifies >> event handling/freeing code, which now does not have to track, which >> packets were copied from the event and which packets should be freed. > > Current spec is stateless. If array is too small in the first call, application must call again with larger array. Application must not proceeding to access packets, before large enough array is used and result event is "freed". I think this can be troublesome for the application. Because if the array is too small, it has to dynamically allocate new array (be it a call to malloc, alloca or just dynamic on-stack arrays with variable size). If API allows the application to do 'partial processing', there would be no dynamic allocation inside the app at this point. Are we OK with dynamic allocation at this point? >> - Do not require to free the event before processing packets. This >> allows one to copy packets from the event in small batches and >> process them accordingly. > > This *disallows* implementation to use the packet as the results event. I remember we talked about ipsec-result-event-as-packet on the previous arch call. I gave it a though. IIRC, your idea was that odp_event_free in this case can just change the type of the event (from IPSEC_RESULT to PACKET). And I had some troubles with this idea. Because now odp_event_free() combines two different code paths for IPSEC_RESULT: - If the event was successfully passed through odp_ipsec_result(), just change the event type. - If it did not pass through odp_ipsec_result() or if the call did not succeed, actually free the event/packet. This introduces extra state into the IPSEC_RESULT context data. > Current spec allows implementation (e.g HW) to enqueue a packet with an IPSEC flag set, and use that as the result event. The conversion function + free can modify the event type from IPSEC result to packet. This is how crypto works today, although the spec is not explicit enough about it. I see your point, I dislike this 'don't actually free' behaviour of odp_event_free() if the odp_ipsec_result() was successful. What if we change the requirements in the following way: If the event was fully processed by odp_ipsec_result(), application may not call odp_event_free() on it. Successful odp_ipsec_result() consumes and frees the event on its own. What do you think? -- With best wishes Dmitry
diff --git a/include/odp/api/spec/ipsec.h b/include/odp/api/spec/ipsec.h index a0ceb11a..8c7750f7 100644 --- a/include/odp/api/spec/ipsec.h +++ b/include/odp/api/spec/ipsec.h @@ -1278,8 +1278,7 @@ int odp_ipsec_out_inline(const odp_ipsec_op_param_t *op_param, * Get IPSEC results from an ODP_EVENT_IPSEC_RESULT event * * Copies IPSEC operation results from an event. The event must be of - * type ODP_EVENT_IPSEC_RESULT. It must be freed before the application passes - * any resulting packet handles to other ODP calls. + * type ODP_EVENT_IPSEC_RESULT. It must be freed via odp_event_free(). * * @param[out] result Pointer to operation result for output. Maybe NULL, if * application is interested only on the number of @@ -1288,7 +1287,8 @@ int odp_ipsec_out_inline(const odp_ipsec_op_param_t *op_param, * * @return Number of packets in the event. If this is larger than * 'result.num_pkt', all packets did not fit into result struct and - * application must call the function again with a larger result struct. + * application must call the function again. Packets returned during + * previous calls will not be returned again in subsequent calls. * @retval <0 On failure * * @see odp_ipsec_in_enq(), odp_ipsec_out_enq()
- Move packets from the event instead of copying them. This simplifies event handling/freeing code, which now does not have to track, which packets were copied from the event and which packets should be freed. - Do not require to free the event before processing packets. This allows one to copy packets from the event in small batches and process them accordingly. Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org> --- include/odp/api/spec/ipsec.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) -- 2.11.0