On 28.04.2017 12:03, Peltonen, Janne (Nokia - FI/Espoo) wrote: > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Bill >> Fischofer >> Sent: Friday, April 28, 2017 4:06 AM >> To: Dmitry Eremin-Solenikov <dmitry.ereminsoleni...@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.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? > > 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
