On Thu, Apr 27, 2017 at 7:50 PM, Dmitry Eremin-Solenikov < dmitry.ereminsoleni...@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.ereminsoleni...@linaro.org > > <mailto:dmitry.ereminsoleni...@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.ereminsoleni...@linaro.org > > <mailto:dmitry.ereminsoleni...@linaro.org> > > > <mailto:dmitry.ereminsoleni...@linaro.org > > <mailto:dmitry.ereminsoleni...@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.ereminsoleni...@linaro.org > > <mailto:dmitry.ereminsoleni...@linaro.org> > > > <mailto:dmitry.ereminsoleni...@linaro.org > > <mailto:dmitry.ereminsoleni...@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 >
