On Mon, Aug 14, 2017 at 11:42 PM, Peltonen, Janne (Nokia - FI/Espoo) < janne.pelto...@nokia.com> wrote:
> > The definition of odp_ipsec_sa_disable() is that RC = 0 means the SA is > now guaranteed idle > > > > It means that no further events for that SA will be posted to the SA queue. > > > > > and may be safely destroyed, > > > > Only from ODP point-of-view. The application may still have IPsec subtype > packets for the SA > > in flight in other threads. > True. Applications must always know themselves. If an application dequeues an event from an SA queue and re-enqueues it elsewhere presumably it's the application's responsibility to track that event as well. > > > > so there would be no other events to dequeue. > > > > There can still be unhandled events in the event queues and outside event > queues in other threads. > > > > My point in my comments is that the application needs to synchronize > between regular IPsec > > completion event handling and destroying an SA and for that an “end > marker” event in the SA > > queue would be quite convenient, or even necessary to avoid more costly > synchronization. > Completion of the disable operation means that the SA is "idle" from an ODP standpoint. As you noted, it's always up to the application to say when it is idle from the application's standpoint. That's why disable is different from destroy. A disabled SA is still valid, it will just not be used by ODP. Applications can still reference (but not initiate work on) disabled SAs. > > > Let’s consider IPsec packet reception in inline mode as an example: > > > > As long as an SA is active (not disabled) incoming packets can match it > and end up as > > IPsec packet events in the SA queue. Thus when an application disables the > SA, there can > > be unhandled events for that SA in the event queues and/or under > processing in other threads. > > If the thread that disabled the SA immediately destroyed the SA, then > event handling > > would attempt to use a destroyed SA (e.g. it would call > odp_ipsec_sa_context()) for an > > SA that was already destroyed, resulting in undefined behavior. > This is a good example. Upon return from odp_ipsec_sa_disable() RC 0 guarantees that ODP will make no further reference to this SA. So no additional packets will match it. But it's still up to the application to decide when to destroy the SA. As far as events go, if an SA event queue is used then RC = 0 says that queue is "idle", not empty, which means that anything ODP wanted to add to that queue has been added and the application is guaranteed that no further events will be added by ODP. So it's a simple matter to drain those events if needed before calling destroy. Note that the async completion means exactly the same, it's just that it's receipt also tells the application that the queue is empty. But again, that's only a statement about this queue. If the application took events off the queue and was still doing something else with them then it's an application responsibility to know when it's safe to issue the destroy call for the SA. > > > If ODP sent SA disable completion event to the SA queue or if the > application did it > > itself after the proposed synchronous SA disable completion, then it would > be able to > > postpone the destroying of an SA until all events for the SA have been > fully handled. > > > Janne > > > > > > *From:* Bill Fischofer [mailto:bill.fischo...@linaro.org] > *Sent:* Monday, August 14, 2017 9:19 PM > *To:* Peltonen, Janne (Nokia - FI/Espoo) <janne.pelto...@nokia.com> > *Cc:* Github ODP bot <odp...@yandex.ru>; lng-odp@lists.linaro.org > *Subject:* Re: [lng-odp] [PATCH API-NEXT v5 1/1] API:IPSEC: IPSEC events > may be reported synchronously. > > > > > > > > On Mon, Aug 14, 2017 at 9:41 AM, Peltonen, Janne (Nokia - FI/Espoo) < > janne.pelto...@nokia.com> wrote: > > > Hi, > > Why is this change needed or preferred over the current API? > > I would guess that many applications that use inline or asynchronous > mode need an end marker in the SA queue so that they know when they > can free their own per-SA state and destroy the ODP SA (i.e. when > all events for the SA being disabled have been dequeued and processed). > > If odp_ipsec_sa_disable() may return synchronously, then such an > application would need to submit an end marker event to the SA queue > itself. The application cannot create an IPsec status event itself so > it has to define another type of event. But the application still has > to be able to process both the IPsec status event and its own event > in the event handler. > > So, based on the above, it does not look like this change would make > the life of an application any easier, but maybe even the opposite. > Does it make ODP implementation significantly easier? > > If this change is anyway made, the API should make it clear that after > return with retval 0 the implementation will not enqueue any further > events for that SA to the SA queue. > > > > The definition of odp_ipsec_sa_disable() is that RC = 0 means the SA is > now guaranteed idle and may be safely destroyed, so there would be no other > events to dequeue. If the implementation cannot guarantee this then it > cannot return synchronously, so I don't see any ambiguity here. > > > > Aside to Maxim: the GitHub to mailing list path has been working well but > the mailing list to GitHub return path is not. Any idea what's needed to > enable that path? Alternatively, Janne, you might want to reply via GitHub > to help keep the discussion in one place along with the PR. > > > > > Janne > > > > > -----Original Message----- > > From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of > Github ODP bot > > Sent: Thursday, August 10, 2017 9:00 AM > > To: lng-odp@lists.linaro.org > > Subject: [lng-odp] [PATCH API-NEXT v5 1/1] API:IPSEC: IPSEC events may > be reported > > synchronously. > > > > From: Nikhil Agarwal <nikhil.agar...@linaro.org> > > > > IPSEC events may be delivered synchronous or ansynchrous > > depending on implementation. Application will know based on > > return value of odp_ipsec_sa_disable API. > > > > Signed-off-by: Nikhil Agarwal <nikhil.agar...@linaro.org> > > --- > > /** Email created from pull request 109 (NikhilA-Linaro:disable_event) > > ** https://github.com/Linaro/odp/pull/109 > > ** Patch: https://github.com/Linaro/odp/pull/109.patch > > ** Base sha: e420668cd3886f003c8bd6022e210bf08a0ee3b5 > > ** Merge commit sha: f65d49b122f2f60a7fc9af3e0a09067dcfd369d9 > > **/ > > include/odp/api/spec/ipsec.h | 17 ++++++++++------- > > 1 file changed, 10 insertions(+), 7 deletions(-) > > > > diff --git a/include/odp/api/spec/ipsec.h b/include/odp/api/spec/ipsec.h > > index 7085bc0d..3f02635a 100644 > > --- a/include/odp/api/spec/ipsec.h > > +++ b/include/odp/api/spec/ipsec.h > > @@ -831,16 +831,19 @@ odp_ipsec_sa_t odp_ipsec_sa_create(const > odp_ipsec_sa_param_t > > *param); > > * before calling disable. Packets in progress during the call may > still match > > * the SA and be processed successfully. > > * > > - * When in synchronous operation mode, the call will return when it's > possible > > - * to destroy the SA. In asynchronous mode, the same is indicated by an > > - * ODP_EVENT_IPSEC_STATUS event sent to the queue specified for the SA. > The > > - * status event is guaranteed to be the last event for the SA, i.e. all > > - * in-progress operations have completed and resulting events > (including status > > - * events) have been enqueued before it. > > + * A return value 0 indicates that the disable request has completed > > + * synchronously and the SA is now disabled. A return value 1 indicates > that the > > + * disable request has been accepted and completion will be indicated > by an > > + * ODP_EVENT_IPSEC_STATUS sent to the queue specified for the SA. This > event is > > + * guaranteed to be the last event for the SA, i.e., all in-progress > operations > > + * have completed and resulting events (including status events) have > been > > + * enqueued before it. In synchronous mode of operation, disable > requests are > > + * gauranteed to complete synchronously as there is no queue assciated > with SA. > > * > > * @param sa IPSEC SA to be disabled > > * > > - * @retval 0 On success > > + * @retval 0 When SA is disabled successfully. > > + * @retval 1 Disable event will be posted on SA queue. > > * @retval <0 On failure > > * > > * @see odp_ipsec_sa_destroy() > > >
