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.fischo...@linaro.org]
Sent: Tuesday, August 15, 2017 3:34 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 11:42 PM, Peltonen, Janne (Nokia - FI/Espoo)
<janne.pelto...@nokia.com<mailto: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<mailto:bill.fischo...@linaro.org>]
Sent: Monday, August 14, 2017 9:19 PM
To: Peltonen, Janne (Nokia - FI/Espoo)
<janne.pelto...@nokia.com<mailto:janne.pelto...@nokia.com>>
Cc: Github ODP bot <odp...@yandex.ru<mailto:odp...@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.pelto...@nokia.com<mailto: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<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<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.agar...@linaro.org<mailto: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<mailto: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()
