| Message ID | 1502344812-23276-2-git-send-email-odpbot@yandex.ru |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [API-NEXT,v5,1/1] API:IPSEC: IPSEC events may be reported synchronously. | expand |
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. Janne > -----Original Message----- > From: lng-odp [mailto:lng-odp-bounces@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.agarwal@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.agarwal@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()
On Mon, Aug 14, 2017 at 9:41 AM, Peltonen, Janne (Nokia - FI/Espoo) < janne.peltonen@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-bounces@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.agarwal@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.agarwal@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() > >
> 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. > 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. 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. 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.fischofer@linaro.org] Sent: Monday, August 14, 2017 9:19 PM To: Peltonen, Janne (Nokia - FI/Espoo) <janne.peltonen@nokia.com> Cc: Github ODP bot <odpbot@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.peltonen@nokia.com<mailto:janne.peltonen@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-bounces@lists.linaro.org<mailto:lng-odp-bounces@lists.linaro.org>] On Behalf Of Github ODP bot > Sent: Thursday, August 10, 2017 9:00 AM > To: lng-odp@lists.linaro.org<mailto: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.agarwal@linaro.org<mailto:nikhil.agarwal@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.agarwal@linaro.org<mailto:nikhil.agarwal@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()
On Mon, Aug 14, 2017 at 11:42 PM, Peltonen, Janne (Nokia - FI/Espoo) < janne.peltonen@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.fischofer@linaro.org] > *Sent:* Monday, August 14, 2017 9:19 PM > *To:* Peltonen, Janne (Nokia - FI/Espoo) <janne.peltonen@nokia.com> > *Cc:* Github ODP bot <odpbot@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.peltonen@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-bounces@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.agarwal@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.agarwal@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() > > >
On 08/15/17 07:42, Peltonen, Janne (Nokia - FI/Espoo) wrote:
> 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.
Looks like script does not work as it should now. Will debug.
Maxim.
replay to test github scripts.
Looks like it's working. Thanks, Maxim! On Tue, Aug 15, 2017 at 11:18 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > replay to test github scripts. >
I think we are pretty much on the same page about the sync issues. It is then just an API design choice whether ODP always sends a disable completion event in async and inline mode (current API) or whether it is left to the application if it needs it (the patch). I was preferring the former, but maybe it is just me. > So it's a simple matter to drain those events if needed before calling destroy. I think an event (enqueued either by ODP or by the application after disabling an SA) is needed to do the draining safely. Maybe a timing based approach would do too with reasonable assumptions about the ODP implementation. > 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. Yes. One way to do it is to configure the SA queue as atomic, another is to use an ordered queue and order the disable completion event handling with respect to other SA events using an ordered lock. Janne From: Bill Fischofer [mailto:bill.fischofer@linaro.org] Sent: Tuesday, August 15, 2017 3:34 PM To: Peltonen, Janne (Nokia - FI/Espoo) <janne.peltonen@nokia.com> Cc: Github ODP bot <odpbot@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 11:42 PM, Peltonen, Janne (Nokia - FI/Espoo) <janne.peltonen@nokia.com<mailto:janne.peltonen@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.fischofer@linaro.org<mailto:bill.fischofer@linaro.org>] Sent: Monday, August 14, 2017 9:19 PM To: Peltonen, Janne (Nokia - FI/Espoo) <janne.peltonen@nokia.com<mailto:janne.peltonen@nokia.com>> Cc: Github ODP bot <odpbot@yandex.ru<mailto:odpbot@yandex.ru>>; lng-odp@lists.linaro.org<mailto: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.peltonen@nokia.com<mailto:janne.peltonen@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-bounces@lists.linaro.org<mailto:lng-odp-bounces@lists.linaro.org>] On Behalf Of Github ODP bot > Sent: Thursday, August 10, 2017 9:00 AM > To: lng-odp@lists.linaro.org<mailto: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.agarwal@linaro.org<mailto:nikhil.agarwal@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.agarwal@linaro.org<mailto:nikhil.agarwal@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()
> -----Original Message----- > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of > Peltonen, Janne (Nokia - FI/Espoo) > Sent: Wednesday, August 16, 2017 9:16 AM > To: Bill Fischofer <bill.fischofer@linaro.org> > Cc: lng-odp@lists.linaro.org > Subject: Suspected SPAM - Re: [lng-odp] [PATCH API-NEXT v5 1/1] API:IPSEC: > IPSEC events may be reported synchronously. > > I think we are pretty much on the same page about the sync issues. It is > then just an API > design choice whether ODP always sends a disable completion event in async > and inline > mode (current API) or whether it is left to the application if it needs it > (the patch). > I was preferring the former, but maybe it is just me. > I also prefer the current API, which is simpler for application (completion signal comes always the same way). > > So it's a simple matter to drain those events if needed before calling > destroy. > > I think an event (enqueued either by ODP or by the application after > disabling an SA) > is needed to do the draining safely. Maybe a timing based approach would > do too with > reasonable assumptions about the ODP implementation. An async application likely needs a marker event anyway, since the same (SA) queue may receive events from different sources (control plane, or multiple SAs) and app could not rely on the queue going empty (in a limited amount of time). -Petri > > > 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. > > Yes. One way to do it is to configure the SA queue as atomic, another is > to use an ordered > queue and order the disable completion event handling with respect to > other SA events > using an ordered lock. > > Janne > > > From: Bill Fischofer [mailto:bill.fischofer@linaro.org] > Sent: Tuesday, August 15, 2017 3:34 PM > To: Peltonen, Janne (Nokia - FI/Espoo) <janne.peltonen@nokia.com> > Cc: Github ODP bot <odpbot@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 11:42 PM, Peltonen, Janne (Nokia - FI/Espoo) > <janne.peltonen@nokia.com<mailto:janne.peltonen@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.fischofer@linaro.org<mailto:bill.fischofer@linaro.org>] > Sent: Monday, August 14, 2017 9:19 PM > To: Peltonen, Janne (Nokia - FI/Espoo) > <janne.peltonen@nokia.com<mailto:janne.peltonen@nokia.com>> > Cc: Github ODP bot <odpbot@yandex.ru<mailto:odpbot@yandex.ru>>; lng- > odp@lists.linaro.org<mailto: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.peltonen@nokia.com<mailto:janne.peltonen@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-bounces@lists.linaro.org<mailto:lng-odp- > bounces@lists.linaro.org>] On Behalf Of Github ODP bot > > Sent: Thursday, August 10, 2017 9:00 AM > > To: lng-odp@lists.linaro.org<mailto: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.agarwal@linaro.org<mailto:nikhil.agarwal@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.agarwal@linaro.org<mailto:nikhil.agarwal@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() >
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()