| Message ID | 20170427115150.19452-3-dmitry.ereminsolenikov@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Apr 27, 2017 at 6:51 AM, Dmitry Eremin-Solenikov < dmitry.ereminsolenikov@linaro.org> wrote: > - 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. > > - Freeing the event in odp_ipsec_result() leaves space for optimized > implementations, where an event is actually a packet with additional > metadata. > > Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org> > --- > include/odp/api/spec/ipsec.h | 22 ++++++++++++++-------- > 1 file changed, 14 insertions(+), 8 deletions(-) > > diff --git a/include/odp/api/spec/ipsec.h b/include/odp/api/spec/ipsec.h > index 7f43e81c..255c5850 100644 > --- a/include/odp/api/spec/ipsec.h > +++ b/include/odp/api/spec/ipsec.h > @@ -1275,17 +1275,23 @@ 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. The event will be freed automatically if > + * odp_ipsec_result() returns 0. In all other case 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 > - * packets. > + * @param[out] result Pointer to operation result for output. May be > + * NULL, if application is interested only on the > Correct typo in original: is interested only *in* the ... > + * number of packets. > * @param event An ODP_EVENT_IPSEC_RESULT event > * > - * @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. > + * @return Number of packets remaining in the event. > + * @retval > 0 All packets did not fit into result struct and > + * application must call the function again. Packets > + * returned during previous calls will not be returned > + * again in subsequent calls. > + * @retval 0 All packets were returned. The event was freed during > + * this call. Application should not access the event > + * afterwards. > * @retval <0 On failure > As we discussed this past week the number of packets returned from a single IPsec operation is expected to be relatively small and bounded, so having the implementation return its upper limit in response to an odp_ipsec_capability() call would enable the application to always provision an odp_ipsec_op_result_t with sufficient entries to accept this maximal return. That avoids the need for stateful processing on the part of both application and implementation. > * > * @see odp_ipsec_in_enq(), odp_ipsec_out_enq() > -- > 2.11.0 > >
On 28.04.2017 01:46, Bill Fischofer wrote: > > > On Thu, Apr 27, 2017 at 6:51 AM, Dmitry Eremin-Solenikov > <dmitry.ereminsolenikov@linaro.org > <mailto:dmitry.ereminsolenikov@linaro.org>> wrote: > > - 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. > > - Freeing the event in odp_ipsec_result() leaves space for optimized > implementations, where an event is actually a packet with additional > metadata. > > Signed-off-by: Dmitry Eremin-Solenikov > <dmitry.ereminsolenikov@linaro.org > <mailto:dmitry.ereminsolenikov@linaro.org>> > --- > include/odp/api/spec/ipsec.h | 22 ++++++++++++++-------- > 1 file changed, 14 insertions(+), 8 deletions(-) > > diff --git a/include/odp/api/spec/ipsec.h b/include/odp/api/spec/ipsec.h > index 7f43e81c..255c5850 100644 > --- a/include/odp/api/spec/ipsec.h > +++ b/include/odp/api/spec/ipsec.h > @@ -1275,17 +1275,23 @@ 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. The event will be freed > automatically if > + * odp_ipsec_result() returns 0. In all other case 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 > - * packets. > + * @param[out] result Pointer to operation result for output. > May be > + * NULL, if application is interested only > on the > > > Correct typo in original: is interested only *in* the ... > > > + * number of packets. > * @param event An ODP_EVENT_IPSEC_RESULT event > * > - * @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. > + * @return Number of packets remaining in the event. > + * @retval > 0 All packets did not fit into result struct and > + * application must call the function again. Packets > + * returned during previous calls will not be returned > + * again in subsequent calls. > + * @retval 0 All packets were returned. The event was freed during > + * this call. Application should not access the event > + * afterwards. > * @retval <0 On failure > > > As we discussed this past week the number of packets returned from a > single IPsec operation is expected to be relatively small and bounded, > so having the implementation return its upper limit in response to an > odp_ipsec_capability() call would enable the application to always > provision an odp_ipsec_op_result_t with sufficient entries to accept > this maximal return. That avoids the need for stateful processing on the > part of both application and implementation. This looks like an artificial limitation from my POV. Basically it would lead to requirement that application writers have to _always_ pass the array of _max_pkt_ elements to odp_ipsec_result(). For some implementations there could be no practical limit on packet amount. -- With best wishes Dmitry
On Thu, Apr 27, 2017 at 5:49 PM, Dmitry Eremin-Solenikov < dmitry.ereminsolenikov@linaro.org> wrote: > On 28.04.2017 01:46, Bill Fischofer wrote: > > > > > > On Thu, Apr 27, 2017 at 6:51 AM, Dmitry Eremin-Solenikov > > <dmitry.ereminsolenikov@linaro.org > > <mailto:dmitry.ereminsolenikov@linaro.org>> wrote: > > > > - 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. > > > > - Freeing the event in odp_ipsec_result() leaves space for optimized > > implementations, where an event is actually a packet with > additional > > metadata. > > > > Signed-off-by: Dmitry Eremin-Solenikov > > <dmitry.ereminsolenikov@linaro.org > > <mailto:dmitry.ereminsolenikov@linaro.org>> > > --- > > include/odp/api/spec/ipsec.h | 22 ++++++++++++++-------- > > 1 file changed, 14 insertions(+), 8 deletions(-) > > > > diff --git a/include/odp/api/spec/ipsec.h > b/include/odp/api/spec/ipsec.h > > index 7f43e81c..255c5850 100644 > > --- a/include/odp/api/spec/ipsec.h > > +++ b/include/odp/api/spec/ipsec.h > > @@ -1275,17 +1275,23 @@ 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. The event will be freed > > automatically if > > + * odp_ipsec_result() returns 0. In all other case 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 > > - * packets. > > + * @param[out] result Pointer to operation result for output. > > May be > > + * NULL, if application is interested only > > on the > > > > > > Correct typo in original: is interested only *in* the ... > > > > > > + * number of packets. > > * @param event An ODP_EVENT_IPSEC_RESULT event > > * > > - * @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. > > + * @return Number of packets remaining in the event. > > + * @retval > 0 All packets did not fit into result struct and > > + * application must call the function again. Packets > > + * returned during previous calls will not be > returned > > + * again in subsequent calls. > > + * @retval 0 All packets were returned. The event was freed > during > > + * this call. Application should not access the event > > + * afterwards. > > * @retval <0 On failure > > > > > > As we discussed this past week the number of packets returned from a > > single IPsec operation is expected to be relatively small and bounded, > > so having the implementation return its upper limit in response to an > > odp_ipsec_capability() call would enable the application to always > > provision an odp_ipsec_op_result_t with sufficient entries to accept > > this maximal return. That avoids the need for stateful processing on the > > part of both application and implementation. > > This looks like an artificial limitation from my POV. Basically it would > lead to requirement that application writers have to _always_ pass the > array of _max_pkt_ elements to odp_ipsec_result(). For some > implementations there could be no practical limit on packet amount. > > I'm not sure I understand how that can be. odp_ipsec_result_t events are associated with IPsec operations that are either explicitly the result of an application odp_ipsec_in/out call or else the result of IPsec packet receipt on an RX interface. Due to possible fragmentation, a single packet could result in multiple output packets (hence more than one packet per odp_ipsec_op_result_t, but the number of fragments resulting from a single input packet is going to be at most a handful. Even allowing that odp_ipsec_in/out can pass an array of packets, the "multiplication" is similarly bounded. Also, shouldn't the signature of this API be: int odp_ipsec_result(odp_ipsec_op_result_t *result, odp_ipsec_result_t ipsec_ev)? It's only valid for odp_ipsec_result_t events so passing a generic event as the 2nd argument seems incorrect here. > > -- > With best wishes > Dmitry >
On 28.04.2017 03:45, Bill Fischofer wrote: > > > On Thu, Apr 27, 2017 at 5:49 PM, Dmitry Eremin-Solenikov > <dmitry.ereminsolenikov@linaro.org > <mailto:dmitry.ereminsolenikov@linaro.org>> wrote: > > On 28.04.2017 01:46, Bill Fischofer wrote: > > > > > > On Thu, Apr 27, 2017 at 6:51 AM, Dmitry Eremin-Solenikov > > <dmitry.ereminsolenikov@linaro.org > <mailto:dmitry.ereminsolenikov@linaro.org> > > <mailto:dmitry.ereminsolenikov@linaro.org > <mailto:dmitry.ereminsolenikov@linaro.org>>> wrote: > > > > - 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. > > > > - Freeing the event in odp_ipsec_result() leaves space for optimized > > implementations, where an event is actually a packet with additional > > metadata. > > > > Signed-off-by: Dmitry Eremin-Solenikov > > <dmitry.ereminsolenikov@linaro.org > <mailto:dmitry.ereminsolenikov@linaro.org> > > <mailto:dmitry.ereminsolenikov@linaro.org > <mailto:dmitry.ereminsolenikov@linaro.org>>> > > --- > > include/odp/api/spec/ipsec.h | 22 ++++++++++++++-------- > > 1 file changed, 14 insertions(+), 8 deletions(-) > > > > diff --git a/include/odp/api/spec/ipsec.h > b/include/odp/api/spec/ipsec.h > > index 7f43e81c..255c5850 100644 > > --- a/include/odp/api/spec/ipsec.h > > +++ b/include/odp/api/spec/ipsec.h > > @@ -1275,17 +1275,23 @@ 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. The event will be freed > > automatically if > > + * odp_ipsec_result() returns 0. In all other case 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 > > - * packets. > > + * @param[out] result Pointer to operation result for output. > > May be > > + * NULL, if application is interested only > > on the > > > > > > Correct typo in original: is interested only *in* the ... > > > > > > + * number of packets. > > * @param event An ODP_EVENT_IPSEC_RESULT event > > * > > - * @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. > > + * @return Number of packets remaining in the event. > > + * @retval > 0 All packets did not fit into result struct and > > + * application must call the function again. > Packets > > + * returned during previous calls will not be > returned > > + * again in subsequent calls. > > + * @retval 0 All packets were returned. The event was > freed during > > + * this call. Application should not access > the event > > + * afterwards. > > * @retval <0 On failure > > > > > > As we discussed this past week the number of packets returned from a > > single IPsec operation is expected to be relatively small and bounded, > > so having the implementation return its upper limit in response to an > > odp_ipsec_capability() call would enable the application to always > > provision an odp_ipsec_op_result_t with sufficient entries to accept > > this maximal return. That avoids the need for stateful processing > on the > > part of both application and implementation. > > This looks like an artificial limitation from my POV. Basically it would > lead to requirement that application writers have to _always_ pass the > array of _max_pkt_ elements to odp_ipsec_result(). For some > implementations there could be no practical limit on packet amount. > > > I'm not sure I understand how that can be. odp_ipsec_result_t events are > associated with IPsec operations that are either explicitly the result > of an application odp_ipsec_in/out call or else the result of IPsec > packet receipt on an RX interface. Due to possible fragmentation, a > single packet could result in multiple output packets (hence more than > one packet per odp_ipsec_op_result_t, but the number of fragments > resulting from a single input packet is going to be at most a handful. > Even allowing that odp_ipsec_in/out can pass an array of packets, the > "multiplication" is similarly bounded. Yes, it is N * in_pkt, but there is no actual limit. If you pass e.g. A input packets, output can be up to N*A packets, not some fixed amount. > > Also, shouldn't the signature of this API be: > > int odp_ipsec_result(odp_ipsec_op_result_t *result, odp_ipsec_result_t > ipsec_ev)? odp_ipsec_result_t is an internal API used to odp_event code to pass event to odp_ipsec_result_* handling. Maybe I should drop it alltogether and replace with just odp_event_t. What do you think? > It's only valid for odp_ipsec_result_t events so passing a generic event > as the 2nd argument seems incorrect here. -- With best wishes Dmitry
On Thu, Apr 27, 2017 at 7:50 PM, Dmitry Eremin-Solenikov < dmitry.ereminsolenikov@linaro.org> wrote: > On 28.04.2017 03:45, Bill Fischofer wrote: > > > > > > On Thu, Apr 27, 2017 at 5:49 PM, Dmitry Eremin-Solenikov > > <dmitry.ereminsolenikov@linaro.org > > <mailto:dmitry.ereminsolenikov@linaro.org>> wrote: > > > > On 28.04.2017 01:46, Bill Fischofer wrote: > > > > > > > > > On Thu, Apr 27, 2017 at 6:51 AM, Dmitry Eremin-Solenikov > > > <dmitry.ereminsolenikov@linaro.org > > <mailto:dmitry.ereminsolenikov@linaro.org> > > > <mailto:dmitry.ereminsolenikov@linaro.org > > <mailto:dmitry.ereminsolenikov@linaro.org>>> wrote: > > > > > > - 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. > > > > > > - Freeing the event in odp_ipsec_result() leaves space for > optimized > > > implementations, where an event is actually a packet with > additional > > > metadata. > > > > > > Signed-off-by: Dmitry Eremin-Solenikov > > > <dmitry.ereminsolenikov@linaro.org > > <mailto:dmitry.ereminsolenikov@linaro.org> > > > <mailto:dmitry.ereminsolenikov@linaro.org > > <mailto:dmitry.ereminsolenikov@linaro.org>>> > > > --- > > > include/odp/api/spec/ipsec.h | 22 ++++++++++++++-------- > > > 1 file changed, 14 insertions(+), 8 deletions(-) > > > > > > diff --git a/include/odp/api/spec/ipsec.h > > b/include/odp/api/spec/ipsec.h > > > index 7f43e81c..255c5850 100644 > > > --- a/include/odp/api/spec/ipsec.h > > > +++ b/include/odp/api/spec/ipsec.h > > > @@ -1275,17 +1275,23 @@ 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. The event will be freed > > > automatically if > > > + * odp_ipsec_result() returns 0. In all other case 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 > > > - * packets. > > > + * @param[out] result Pointer to operation result for > output. > > > May be > > > + * NULL, if application is interested > only > > > on the > > > > > > > > > Correct typo in original: is interested only *in* the ... > > > > > > > > > + * number of packets. > > > * @param event An ODP_EVENT_IPSEC_RESULT event > > > * > > > - * @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. > > > + * @return Number of packets remaining in the event. > > > + * @retval > 0 All packets did not fit into result struct > and > > > + * application must call the function again. > > Packets > > > + * returned during previous calls will not be > > returned > > > + * again in subsequent calls. > > > + * @retval 0 All packets were returned. The event was > > freed during > > > + * this call. Application should not access > > the event > > > + * afterwards. > > > * @retval <0 On failure > > > > > > > > > As we discussed this past week the number of packets returned from > a > > > single IPsec operation is expected to be relatively small and > bounded, > > > so having the implementation return its upper limit in response to > an > > > odp_ipsec_capability() call would enable the application to always > > > provision an odp_ipsec_op_result_t with sufficient entries to > accept > > > this maximal return. That avoids the need for stateful processing > > on the > > > part of both application and implementation. > > > > This looks like an artificial limitation from my POV. Basically it > would > > lead to requirement that application writers have to _always_ pass > the > > array of _max_pkt_ elements to odp_ipsec_result(). For some > > implementations there could be no practical limit on packet amount. > > > > > > I'm not sure I understand how that can be. odp_ipsec_result_t events are > > associated with IPsec operations that are either explicitly the result > > of an application odp_ipsec_in/out call or else the result of IPsec > > packet receipt on an RX interface. Due to possible fragmentation, a > > single packet could result in multiple output packets (hence more than > > one packet per odp_ipsec_op_result_t, but the number of fragments > > resulting from a single input packet is going to be at most a handful. > > Even allowing that odp_ipsec_in/out can pass an array of packets, the > > "multiplication" is similarly bounded. > > Yes, it is N * in_pkt, but there is no actual limit. If you pass e.g. A > input packets, output can be up to N*A packets, not some fixed amount. > Perhaps the output from odp_ipsec_capability() then would be N and the application can use this to size the max number of return packets since it controls A? > > > > > Also, shouldn't the signature of this API be: > > > > int odp_ipsec_result(odp_ipsec_op_result_t *result, odp_ipsec_result_t > > ipsec_ev)? > > odp_ipsec_result_t is an internal API used to odp_event code to pass > event to odp_ipsec_result_* handling. Maybe I should drop it alltogether > and replace with just odp_event_t. What do you think? > odp_event_t is a generic type that encompasses any specific event so that odp_queue_enq/deq() don't have to be generic functions (since generic functions aren't supported in C99). The events that come out of IPsec operations are odp_ipsec_result_t events so best to use that here, no? > > > It's only valid for odp_ipsec_result_t events so passing a generic event > > as the 2nd argument seems incorrect here. > > > -- > With best wishes > Dmitry >
> -----Original Message----- > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Bill Fischofer > Sent: Friday, April 28, 2017 4:06 AM > To: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org> > Cc: lng-odp-forward <lng-odp@lists.linaro.org> > Subject: Re: [lng-odp] [[RFCv2] 3/4] api: ipsec: change semantics of odp_ipsec_result > function > > On Thu, Apr 27, 2017 at 7:50 PM, Dmitry Eremin-Solenikov < > dmitry.ereminsolenikov@linaro.org> wrote: > > > On 28.04.2017 03:45, Bill Fischofer wrote: > > > > > > > > > On Thu, Apr 27, 2017 at 5:49 PM, Dmitry Eremin-Solenikov > > > <dmitry.ereminsolenikov@linaro.org > > > <mailto:dmitry.ereminsolenikov@linaro.org>> wrote: > > > > > > On 28.04.2017 01:46, Bill Fischofer wrote: > > > > > > > > > > > > On Thu, Apr 27, 2017 at 6:51 AM, Dmitry Eremin-Solenikov > > > > <dmitry.ereminsolenikov@linaro.org > > > <mailto:dmitry.ereminsolenikov@linaro.org> > > > > <mailto:dmitry.ereminsolenikov@linaro.org > > > <mailto:dmitry.ereminsolenikov@linaro.org>>> wrote: > > > > > > > > - 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. > > > > > > > > - Freeing the event in odp_ipsec_result() leaves space for > > optimized > > > > implementations, where an event is actually a packet with > > additional > > > > metadata. > > > > > > > > Signed-off-by: Dmitry Eremin-Solenikov > > > > <dmitry.ereminsolenikov@linaro.org > > > <mailto:dmitry.ereminsolenikov@linaro.org> > > > > <mailto:dmitry.ereminsolenikov@linaro.org > > > <mailto:dmitry.ereminsolenikov@linaro.org>>> > > > > --- > > > > include/odp/api/spec/ipsec.h | 22 ++++++++++++++-------- > > > > 1 file changed, 14 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/include/odp/api/spec/ipsec.h > > > b/include/odp/api/spec/ipsec.h > > > > index 7f43e81c..255c5850 100644 > > > > --- a/include/odp/api/spec/ipsec.h > > > > +++ b/include/odp/api/spec/ipsec.h > > > > @@ -1275,17 +1275,23 @@ 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. The event will be freed > > > > automatically if > > > > + * odp_ipsec_result() returns 0. In all other case 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 > > > > - * packets. > > > > + * @param[out] result Pointer to operation result for > > output. > > > > May be > > > > + * NULL, if application is interested > > only > > > > on the > > > > > > > > > > > > Correct typo in original: is interested only *in* the ... > > > > > > > > > > > > + * number of packets. > > > > * @param event An ODP_EVENT_IPSEC_RESULT event > > > > * > > > > - * @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. > > > > + * @return Number of packets remaining in the event. > > > > + * @retval > 0 All packets did not fit into result struct > > and > > > > + * application must call the function again. > > > Packets > > > > + * returned during previous calls will not be > > > returned > > > > + * again in subsequent calls. > > > > + * @retval 0 All packets were returned. The event was > > > freed during > > > > + * this call. Application should not access > > > the event > > > > + * afterwards. > > > > * @retval <0 On failure > > > > > > > > > > > > As we discussed this past week the number of packets returned from > > a > > > > single IPsec operation is expected to be relatively small and > > bounded, > > > > so having the implementation return its upper limit in response to > > an > > > > odp_ipsec_capability() call would enable the application to always > > > > provision an odp_ipsec_op_result_t with sufficient entries to > > accept > > > > this maximal return. That avoids the need for stateful processing > > > on the > > > > part of both application and implementation. > > > > > > This looks like an artificial limitation from my POV. Basically it > > would > > > lead to requirement that application writers have to _always_ pass > > the > > > array of _max_pkt_ elements to odp_ipsec_result(). For some > > > implementations there could be no practical limit on packet amount. > > > > > > > > > I'm not sure I understand how that can be. odp_ipsec_result_t events are > > > associated with IPsec operations that are either explicitly the result > > > of an application odp_ipsec_in/out call or else the result of IPsec > > > packet receipt on an RX interface. Due to possible fragmentation, a > > > single packet could result in multiple output packets (hence more than > > > one packet per odp_ipsec_op_result_t, but the number of fragments > > > resulting from a single input packet is going to be at most a handful. > > > Even allowing that odp_ipsec_in/out can pass an array of packets, the > > > "multiplication" is similarly bounded. > > > > Yes, it is N * in_pkt, but there is no actual limit. If you pass e.g. A > > input packets, output can be up to N*A packets, not some fixed amount. > > > > Perhaps the output from odp_ipsec_capability() then would be N and the > application can use this to size the max number of return packets since it > controls A? There is not necessarily such correspondence between the enqueue operations and the resulting events. The current API allows one result event combine the results from multiple enqueue operations or split the results of one operation to multiple events. Also, in case of inline processing there are no input operations, just the events that arrive. One possibility would be to specify that there is only one event per input packet so that there would be only one packet in the result event, except in case of fragmentation offload. Combined with some upper limit on the max number of fragments this would avoid dynamic memory allocation for the event handling in the run time. But this might sacrifice some performance? In the current API one might have to do a couple of dynamic memory allocations during run time until the result buffer has grown big enough for all subsequent events. > > > > > > > > > > Also, shouldn't the signature of this API be: > > > > > > int odp_ipsec_result(odp_ipsec_op_result_t *result, odp_ipsec_result_t > > > ipsec_ev)? > > > > odp_ipsec_result_t is an internal API used to odp_event code to pass > > event to odp_ipsec_result_* handling. Maybe I should drop it alltogether > > and replace with just odp_event_t. What do you think? > > > > odp_event_t is a generic type that encompasses any specific event so that > odp_queue_enq/deq() don't have to be generic functions (since generic > functions aren't supported in C99). The events that come out of IPsec > operations are odp_ipsec_result_t events so best to use that here, no? > In the ODP API there are only generic events from which one can get odp_ipsec_op_result_t objects through this function and that is why ipsec_ev should be odp_event_t here. odp_ipsec_result_t is an internal type used in this implementation where ipsec events happen to be of that type. In other implementations they may be equal to other types. But from application point of view ipsec events are opaque (and thus odp_event_t is all one needs) and need to be interpreted through this API function. > > > > > > It's only valid for odp_ipsec_result_t events so passing a generic event > > > as the 2nd argument seems incorrect here. It would be more correct to say that the function is valid only for events of type ODP_EVENT_IPSEC_RESULT. At API level there is no separate type defined as it would not be very useful. Janne
On 28.04.2017 12:03, Peltonen, Janne (Nokia - FI/Espoo) wrote: > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Bill Fischofer >> Sent: Friday, April 28, 2017 4:06 AM >> To: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org> >> Cc: lng-odp-forward <lng-odp@lists.linaro.org> >> Subject: Re: [lng-odp] [[RFCv2] 3/4] api: ipsec: change semantics of odp_ipsec_result >> function >> >> On Thu, Apr 27, 2017 at 7:50 PM, Dmitry Eremin-Solenikov < >> dmitry.ereminsolenikov@linaro.org> wrote: >> >>> On 28.04.2017 03:45, Bill Fischofer wrote: >>>> >>>> >>>> On Thu, Apr 27, 2017 at 5:49 PM, Dmitry Eremin-Solenikov >>>> <dmitry.ereminsolenikov@linaro.org >>>> <mailto:dmitry.ereminsolenikov@linaro.org>> wrote: >>>> >>>> On 28.04.2017 01:46, Bill Fischofer wrote: >>>> > >>>> > >>>> > On Thu, Apr 27, 2017 at 6:51 AM, Dmitry Eremin-Solenikov >>>> > <dmitry.ereminsolenikov@linaro.org >>>> <mailto:dmitry.ereminsolenikov@linaro.org> >>>> > <mailto:dmitry.ereminsolenikov@linaro.org >>>> <mailto:dmitry.ereminsolenikov@linaro.org>>> wrote: >>>> Also, shouldn't the signature of this API be: >>>> >>>> int odp_ipsec_result(odp_ipsec_op_result_t *result, odp_ipsec_result_t >>>> ipsec_ev)? >>> >>> odp_ipsec_result_t is an internal API used to odp_event code to pass >>> event to odp_ipsec_result_* handling. Maybe I should drop it alltogether >>> and replace with just odp_event_t. What do you think? >>> >> >> odp_event_t is a generic type that encompasses any specific event so that >> odp_queue_enq/deq() don't have to be generic functions (since generic >> functions aren't supported in C99). The events that come out of IPsec >> operations are odp_ipsec_result_t events so best to use that here, no? >> > > In the ODP API there are only generic events from which one can get > odp_ipsec_op_result_t objects through this function and that is why > ipsec_ev should be odp_event_t here. > > odp_ipsec_result_t is an internal type used in this implementation > where ipsec events happen to be of that type. In other implementations > they may be equal to other types. But from application point of view > ipsec events are opaque (and thus odp_event_t is all one needs) and > need to be interpreted through this API function. Most probably we should also make odp_crypto_* API to use odp_event_t and hide odp_crypto_compl_t as internal API. > It would be more correct to say that the function is valid only > for events of type ODP_EVENT_IPSEC_RESULT. At API level there is > no separate type defined as it would not be very useful. It is stated so in API docs. -- With best wishes Dmitry
On 28.04.2017 12:03, Peltonen, Janne (Nokia - FI/Espoo) wrote: > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Bill Fischofer >> Sent: Friday, April 28, 2017 4:06 AM >> To: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org> >> Cc: lng-odp-forward <lng-odp@lists.linaro.org> >> Subject: Re: [lng-odp] [[RFCv2] 3/4] api: ipsec: change semantics of odp_ipsec_result >> function >> >> On Thu, Apr 27, 2017 at 7:50 PM, Dmitry Eremin-Solenikov < >> dmitry.ereminsolenikov@linaro.org> wrote: >> >>> On 28.04.2017 03:45, Bill Fischofer wrote: >>>> >>>> >>>> On Thu, Apr 27, 2017 at 5:49 PM, Dmitry Eremin-Solenikov >>>> <dmitry.ereminsolenikov@linaro.org >>>> <mailto:dmitry.ereminsolenikov@linaro.org>> wrote: >>>> >>>> On 28.04.2017 01:46, Bill Fischofer wrote: >>>> > >>>> > >>>> > On Thu, Apr 27, 2017 at 6:51 AM, Dmitry Eremin-Solenikov >>>> > <dmitry.ereminsolenikov@linaro.org >>>> <mailto:dmitry.ereminsolenikov@linaro.org> >>>> > <mailto:dmitry.ereminsolenikov@linaro.org >>>> <mailto:dmitry.ereminsolenikov@linaro.org>>> wrote: >>>> > >>>> > - 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. >>>> > >>>> > - Freeing the event in odp_ipsec_result() leaves space for >>> optimized >>>> > implementations, where an event is actually a packet with >>> additional >>>> > metadata. >>>> > >>>> > Signed-off-by: Dmitry Eremin-Solenikov >>>> > <dmitry.ereminsolenikov@linaro.org >>>> <mailto:dmitry.ereminsolenikov@linaro.org> >>>> > <mailto:dmitry.ereminsolenikov@linaro.org >>>> <mailto:dmitry.ereminsolenikov@linaro.org>>> >>>> > --- >>>> > include/odp/api/spec/ipsec.h | 22 ++++++++++++++-------- >>>> > 1 file changed, 14 insertions(+), 8 deletions(-) >>>> > >>>> > diff --git a/include/odp/api/spec/ipsec.h >>>> b/include/odp/api/spec/ipsec.h >>>> > index 7f43e81c..255c5850 100644 >>>> > --- a/include/odp/api/spec/ipsec.h >>>> > +++ b/include/odp/api/spec/ipsec.h >>>> > @@ -1275,17 +1275,23 @@ 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. The event will be freed >>>> > automatically if >>>> > + * odp_ipsec_result() returns 0. In all other case 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 >>>> > - * packets. >>>> > + * @param[out] result Pointer to operation result for >>> output. >>>> > May be >>>> > + * NULL, if application is interested >>> only >>>> > on the >>>> > >>>> > >>>> > Correct typo in original: is interested only *in* the ... >>>> > >>>> > >>>> > + * number of packets. >>>> > * @param event An ODP_EVENT_IPSEC_RESULT event >>>> > * >>>> > - * @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. >>>> > + * @return Number of packets remaining in the event. >>>> > + * @retval > 0 All packets did not fit into result struct >>> and >>>> > + * application must call the function again. >>>> Packets >>>> > + * returned during previous calls will not be >>>> returned >>>> > + * again in subsequent calls. >>>> > + * @retval 0 All packets were returned. The event was >>>> freed during >>>> > + * this call. Application should not access >>>> the event >>>> > + * afterwards. >>>> > * @retval <0 On failure >>>> > >>>> > >>>> > As we discussed this past week the number of packets returned from >>> a >>>> > single IPsec operation is expected to be relatively small and >>> bounded, >>>> > so having the implementation return its upper limit in response to >>> an >>>> > odp_ipsec_capability() call would enable the application to always >>>> > provision an odp_ipsec_op_result_t with sufficient entries to >>> accept >>>> > this maximal return. That avoids the need for stateful processing >>>> on the >>>> > part of both application and implementation. >>>> >>>> This looks like an artificial limitation from my POV. Basically it >>> would >>>> lead to requirement that application writers have to _always_ pass >>> the >>>> array of _max_pkt_ elements to odp_ipsec_result(). For some >>>> implementations there could be no practical limit on packet amount. >>>> >>>> >>>> I'm not sure I understand how that can be. odp_ipsec_result_t events are >>>> associated with IPsec operations that are either explicitly the result >>>> of an application odp_ipsec_in/out call or else the result of IPsec >>>> packet receipt on an RX interface. Due to possible fragmentation, a >>>> single packet could result in multiple output packets (hence more than >>>> one packet per odp_ipsec_op_result_t, but the number of fragments >>>> resulting from a single input packet is going to be at most a handful. >>>> Even allowing that odp_ipsec_in/out can pass an array of packets, the >>>> "multiplication" is similarly bounded. >>> >>> Yes, it is N * in_pkt, but there is no actual limit. If you pass e.g. A >>> input packets, output can be up to N*A packets, not some fixed amount. >>> >> >> Perhaps the output from odp_ipsec_capability() then would be N and the >> application can use this to size the max number of return packets since it >> controls A? > > There is not necessarily such correspondence between the enqueue operations > and the resulting events. The current API allows one result event combine > the results from multiple enqueue operations or split the results of one > operation to multiple events. Also, in case of inline processing there are > no input operations, just the events that arrive. > > One possibility would be to specify that there is only one event per input > packet so that there would be only one packet in the result event, except > in case of fragmentation offload. Combined with some upper limit on the > max number of fragments this would avoid dynamic memory allocation for > the event handling in the run time. But this might sacrifice some performance? > > In the current API one might have to do a couple of dynamic memory allocations > during run time until the result buffer has grown big enough for all > subsequent events. Ok. To summarize the discussion. Are the any arguments why copy-out API is worse than capability? Than dynamic reallocation? Also, if odp_ipsec_result() retuned value which is greater than num_pkt, did it fill those packets and results in the structure? Maybe it is more of a question to hardware teams (Bala, Nikhil)? -- With best wishes Dmitry
On Fri, Apr 28, 2017 at 5:44 AM, Dmitry Eremin-Solenikov < dmitry.ereminsolenikov@linaro.org> wrote: > On 28.04.2017 12:03, Peltonen, Janne (Nokia - FI/Espoo) wrote: > > > >> -----Original Message----- > >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of > Bill Fischofer > >> Sent: Friday, April 28, 2017 4:06 AM > >> To: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org> > >> Cc: lng-odp-forward <lng-odp@lists.linaro.org> > >> Subject: Re: [lng-odp] [[RFCv2] 3/4] api: ipsec: change semantics of > odp_ipsec_result > >> function > >> > >> On Thu, Apr 27, 2017 at 7:50 PM, Dmitry Eremin-Solenikov < > >> dmitry.ereminsolenikov@linaro.org> wrote: > >> > >>> On 28.04.2017 03:45, Bill Fischofer wrote: > >>>> > >>>> > >>>> On Thu, Apr 27, 2017 at 5:49 PM, Dmitry Eremin-Solenikov > >>>> <dmitry.ereminsolenikov@linaro.org > >>>> <mailto:dmitry.ereminsolenikov@linaro.org>> wrote: > >>>> > >>>> On 28.04.2017 01:46, Bill Fischofer wrote: > >>>> > > >>>> > > >>>> > On Thu, Apr 27, 2017 at 6:51 AM, Dmitry Eremin-Solenikov > >>>> > <dmitry.ereminsolenikov@linaro.org > >>>> <mailto:dmitry.ereminsolenikov@linaro.org> > >>>> > <mailto:dmitry.ereminsolenikov@linaro.org > >>>> <mailto:dmitry.ereminsolenikov@linaro.org>>> wrote: > > >>>> Also, shouldn't the signature of this API be: > >>>> > >>>> int odp_ipsec_result(odp_ipsec_op_result_t *result, > odp_ipsec_result_t > >>>> ipsec_ev)? > >>> > >>> odp_ipsec_result_t is an internal API used to odp_event code to pass > >>> event to odp_ipsec_result_* handling. Maybe I should drop it > alltogether > >>> and replace with just odp_event_t. What do you think? > >>> > >> > >> odp_event_t is a generic type that encompasses any specific event so > that > >> odp_queue_enq/deq() don't have to be generic functions (since generic > >> functions aren't supported in C99). The events that come out of IPsec > >> operations are odp_ipsec_result_t events so best to use that here, no? > >> > > > > In the ODP API there are only generic events from which one can get > > odp_ipsec_op_result_t objects through this function and that is why > > ipsec_ev should be odp_event_t here. > > > > odp_ipsec_result_t is an internal type used in this implementation > > where ipsec events happen to be of that type. In other implementations > > they may be equal to other types. But from application point of view > > ipsec events are opaque (and thus odp_event_t is all one needs) and > > need to be interpreted through this API function. > > Most probably we should also make odp_crypto_* API to use odp_event_t > and hide odp_crypto_compl_t as internal API. > Packets also arrive as ODP events, but the odp_packet_xxx() APIs take odp_packet_t objects, not odp_event_t. The same for odp_buffer_xxx() APIs taking odp_buffer_t. odp_timeout_xxx() APIs taking odp_timeout_t, and odp_crypto_xxx() APIs taking odp_crypto_xxx_ events. This is an odp_ipsec_xxx API, not an odp_event_xxx API hence it should take an odp_ipsec_xxx type for consistency. > > > It would be more correct to say that the function is valid only > > for events of type ODP_EVENT_IPSEC_RESULT. At API level there is > > no separate type defined as it would not be very useful. > > It is stated so in API docs. > The consistent way to state this (as is done for other APIs) is to have it take an odp_ipsec_xxx type and note that the odp_ipsec_result_from_event() call is only meaningful when given events of type ODP_EVENT_IPSEC_RESULT. > > -- > With best wishes > Dmitry >
Bill Fischofer [mailto:bill.fischofer@linaro.org] wrote: > On Fri, Apr 28, 2017 at 5:44 AM, Dmitry Eremin-Solenikov < > dmitry.ereminsolenikov at linaro.org> wrote: > > > On 28.04.2017 12:03, Peltonen, Janne (Nokia - FI/Espoo) wrote: > > > > > >> -----Original Message----- > > >> From: lng-odp [mailto:lng-odp-bounces at lists.linaro.org] On Behalf Of > > Bill Fischofer > > >> Sent: Friday, April 28, 2017 4:06 AM > > >> To: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov at linaro.org> > > >> Cc: lng-odp-forward <lng-odp at lists.linaro.org> > > >> Subject: Re: [lng-odp] [[RFCv2] 3/4] api: ipsec: change semantics of > > odp_ipsec_result > > >> function > > >> > > >> On Thu, Apr 27, 2017 at 7:50 PM, Dmitry Eremin-Solenikov < > > >> dmitry.ereminsolenikov at linaro.org> wrote: > > >> > > >>> On 28.04.2017 03:45, Bill Fischofer wrote: > > >>>> > > >>>> > > >>>> On Thu, Apr 27, 2017 at 5:49 PM, Dmitry Eremin-Solenikov > > >>>> <dmitry.ereminsolenikov at linaro.org > > >>>> <mailto:dmitry.ereminsolenikov at linaro.org>> wrote: > > >>>> > > >>>> On 28.04.2017 01:46, Bill Fischofer wrote: > > >>>> > > > >>>> > > > >>>> > On Thu, Apr 27, 2017 at 6:51 AM, Dmitry Eremin-Solenikov > > >>>> > <dmitry.ereminsolenikov at linaro.org > > >>>> <mailto:dmitry.ereminsolenikov at linaro.org> > > >>>> > <mailto:dmitry.ereminsolenikov at linaro.org > > >>>> <mailto:dmitry.ereminsolenikov at linaro.org>>> wrote: > > > > >>>> Also, shouldn't the signature of this API be: > > >>>> > > >>>> int odp_ipsec_result(odp_ipsec_op_result_t *result, > > odp_ipsec_result_t > > >>>> ipsec_ev)? > > >>> > > >>> odp_ipsec_result_t is an internal API used to odp_event code to pass > > >>> event to odp_ipsec_result_* handling. Maybe I should drop it > > alltogether > > >>> and replace with just odp_event_t. What do you think? > > >>> > > >> > > >> odp_event_t is a generic type that encompasses any specific event so > > that > > >> odp_queue_enq/deq() don't have to be generic functions (since generic > > >> functions aren't supported in C99). The events that come out of IPsec > > >> operations are odp_ipsec_result_t events so best to use that here, no? > > >> > > > > > > In the ODP API there are only generic events from which one can get > > > odp_ipsec_op_result_t objects through this function and that is why > > > ipsec_ev should be odp_event_t here. > > > > > > odp_ipsec_result_t is an internal type used in this implementation > > > where ipsec events happen to be of that type. In other implementations > > > they may be equal to other types. But from application point of view > > > ipsec events are opaque (and thus odp_event_t is all one needs) and > > > need to be interpreted through this API function. > > > > Most probably we should also make odp_crypto_* API to use odp_event_t > > and hide odp_crypto_compl_t as internal API. > > > > Packets also arrive as ODP events, but the odp_packet_xxx() APIs take > odp_packet_t objects, not odp_event_t. The same for odp_buffer_xxx() APIs > taking odp_buffer_t. odp_timeout_xxx() APIs taking odp_timeout_t, and > odp_crypto_xxx() APIs taking odp_crypto_xxx_ events. This is an > odp_ipsec_xxx API, not an odp_event_xxx API hence it should take an > odp_ipsec_xxx type for consistency. > I think I misunderstood the original comment. So the odp_ipsec_xxx type would also be opaque to the application and would still have to be converted through the odp_ipsec_result() function to a non-opaque type (odp_ipsec_op_result_t). That sounds reasonable to me and looks consistent with the crypto API. There are currently two IPsec specific event types, so maybe two new opaque types would be needed. Maybe Petri wants to chime in and comment. > > > > > > It would be more correct to say that the function is valid only > > > for events of type ODP_EVENT_IPSEC_RESULT. At API level there is > > > no separate type defined as it would not be very useful. > > > > It is stated so in API docs. > > > > The consistent way to state this (as is done for other APIs) is to have it > take an odp_ipsec_xxx type and note that the odp_ipsec_result_from_event() > call is only meaningful when given events of type ODP_EVENT_IPSEC_RESULT. > > > > > > -- > > With best wishes > > Dmitry > >
On Fri, Apr 28, 2017 at 9:58 AM, Peltonen, Janne (Nokia - FI/Espoo) < janne.peltonen@nokia.com> wrote: > > Bill Fischofer [mailto:bill.fischofer@linaro.org] wrote: > > On Fri, Apr 28, 2017 at 5:44 AM, Dmitry Eremin-Solenikov < > > dmitry.ereminsolenikov at linaro.org> wrote: > > > > > On 28.04.2017 12:03, Peltonen, Janne (Nokia - FI/Espoo) wrote: > > > > > > > >> -----Original Message----- > > > >> From: lng-odp [mailto:lng-odp-bounces at lists.linaro.org] On > Behalf Of > > > Bill Fischofer > > > >> Sent: Friday, April 28, 2017 4:06 AM > > > >> To: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov at linaro.org> > > > >> Cc: lng-odp-forward <lng-odp at lists.linaro.org> > > > >> Subject: Re: [lng-odp] [[RFCv2] 3/4] api: ipsec: change semantics of > > > odp_ipsec_result > > > >> function > > > >> > > > >> On Thu, Apr 27, 2017 at 7:50 PM, Dmitry Eremin-Solenikov < > > > >> dmitry.ereminsolenikov at linaro.org> wrote: > > > >> > > > >>> On 28.04.2017 03:45, Bill Fischofer wrote: > > > >>>> > > > >>>> > > > >>>> On Thu, Apr 27, 2017 at 5:49 PM, Dmitry Eremin-Solenikov > > > >>>> <dmitry.ereminsolenikov at linaro.org > > > >>>> <mailto:dmitry.ereminsolenikov at linaro.org>> wrote: > > > >>>> > > > >>>> On 28.04.2017 01:46, Bill Fischofer wrote: > > > >>>> > > > > >>>> > > > > >>>> > On Thu, Apr 27, 2017 at 6:51 AM, Dmitry Eremin-Solenikov > > > >>>> > <dmitry.ereminsolenikov at linaro.org > > > >>>> <mailto:dmitry.ereminsolenikov at linaro.org> > > > >>>> > <mailto:dmitry.ereminsolenikov at linaro.org > > > >>>> <mailto:dmitry.ereminsolenikov at linaro.org>>> wrote: > > > > > > >>>> Also, shouldn't the signature of this API be: > > > >>>> > > > >>>> int odp_ipsec_result(odp_ipsec_op_result_t *result, > > > odp_ipsec_result_t > > > >>>> ipsec_ev)? > > > >>> > > > >>> odp_ipsec_result_t is an internal API used to odp_event code to > pass > > > >>> event to odp_ipsec_result_* handling. Maybe I should drop it > > > alltogether > > > >>> and replace with just odp_event_t. What do you think? > > > >>> > > > >> > > > >> odp_event_t is a generic type that encompasses any specific event so > > > that > > > >> odp_queue_enq/deq() don't have to be generic functions (since > generic > > > >> functions aren't supported in C99). The events that come out of > IPsec > > > >> operations are odp_ipsec_result_t events so best to use that here, > no? > > > >> > > > > > > > > In the ODP API there are only generic events from which one can get > > > > odp_ipsec_op_result_t objects through this function and that is why > > > > ipsec_ev should be odp_event_t here. > > > > > > > > odp_ipsec_result_t is an internal type used in this implementation > > > > where ipsec events happen to be of that type. In other > implementations > > > > they may be equal to other types. But from application point of view > > > > ipsec events are opaque (and thus odp_event_t is all one needs) and > > > > need to be interpreted through this API function. > > > > > > Most probably we should also make odp_crypto_* API to use odp_event_t > > > and hide odp_crypto_compl_t as internal API. > > > > > > > Packets also arrive as ODP events, but the odp_packet_xxx() APIs take > > odp_packet_t objects, not odp_event_t. The same for odp_buffer_xxx() APIs > > taking odp_buffer_t. odp_timeout_xxx() APIs taking odp_timeout_t, and > > odp_crypto_xxx() APIs taking odp_crypto_xxx_ events. This is an > > odp_ipsec_xxx API, not an odp_event_xxx API hence it should take an > > odp_ipsec_xxx type for consistency. > > > > I think I misunderstood the original comment. So the odp_ipsec_xxx type > would also be opaque to the application and would still have to be > converted through the odp_ipsec_result() function to a non-opaque > type (odp_ipsec_op_result_t). That sounds reasonable to me and looks > consistent with the crypto API. There are currently two IPsec specific > event types, so maybe two new opaque types would be needed. > Yes, all event types are opaque which is why we have the various odp_xxx_to/from_event() APIs to handle the conversions. Again, this is because C99 does not support generic functions. Otherwise we'd just pass these various type-specific events to a common odp_event_free() routine and also allow odp_queue_enq() to accept them and odp_schedule() to produce them. > > Maybe Petri wants to chime in and comment. > I'll put this on the agenda for Monday's ARCH call to discuss and hopefully get closure on this topic. > > > > > > > > > > It would be more correct to say that the function is valid only > > > > for events of type ODP_EVENT_IPSEC_RESULT. At API level there is > > > > no separate type defined as it would not be very useful. > > > > > > It is stated so in API docs. > > > > > > > The consistent way to state this (as is done for other APIs) is to have > it > > take an odp_ipsec_xxx type and note that the > odp_ipsec_result_from_event() > > call is only meaningful when given events of type ODP_EVENT_IPSEC_RESULT. > > > > > > > > > > -- > > > With best wishes > > > Dmitry > > > > >
diff --git a/include/odp/api/spec/ipsec.h b/include/odp/api/spec/ipsec.h index 7f43e81c..255c5850 100644 --- a/include/odp/api/spec/ipsec.h +++ b/include/odp/api/spec/ipsec.h @@ -1275,17 +1275,23 @@ 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. The event will be freed automatically if + * odp_ipsec_result() returns 0. In all other case 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 - * packets. + * @param[out] result Pointer to operation result for output. May be + * NULL, if application is interested only on the + * number of packets. * @param event An ODP_EVENT_IPSEC_RESULT event * - * @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. + * @return Number of packets remaining in the event. + * @retval > 0 All packets did not fit into result struct and + * application must call the function again. Packets + * returned during previous calls will not be returned + * again in subsequent calls. + * @retval 0 All packets were returned. The event was freed during + * this call. Application should not access the event + * afterwards. * @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. - Freeing the event in odp_ipsec_result() leaves space for optimized implementations, where an event is actually a packet with additional metadata. Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org> --- include/odp/api/spec/ipsec.h | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) -- 2.11.0