Bill Fischofer [mailto:bill.fischo...@linaro.org] wrote:
> On Fri, Sep 8, 2017 at 6:10 AM, Janne Peltonen <janne.pelto...@nokia.com>
> wrote:
> >
> >
> > On Fri, 8 Sep 2017, Nikhil Agarwal wrote:
> >> On 7 September 2017 at 14:09, Peltonen, Janne (Nokia - FI/Espoo)
> >> <janne.pelto...@nokia.com> wrote:>
> >> > Bill Fischofer wrote:
> >> > > if (odp_ipsec_result(&ipsec_res, pkt) < 0) { /* Stale
> >> > > event, discard */
> >>
> >> > There is a race condition here if the idea is that calling
> >> > odp_ipsec_sa_disable() (or destroy()) marks the SA stale to prevent
> >> > further processing for that SA in the result event handling.
> >>
> >> > Maybe the odp_ipsec_result() succeeds but the SA becomes stale
> >> > right after that, before rest of the processing is done.
> >>
> >> Same race condition exist in the proposed API when one thread received the
> >> last
> >> packet of SA and still process it while the other thread on receiving
> >> disable
> >> event destroy the SA, application will always need to synchronize its own
> >> thread.
> >
> > There is no race in the API, only in incorrect use of it. As I explained,
> > synchronization is needed even with the disable event and it can be
> > easily done using an ordered lock that ensures that other event handling
> > threads are finished processing the SA (they have released the queue
> > context through another call to schedule or explicitly). So the event
> > handler for the disable event can destroy the SA safely without any race
> > if it uses an ordered lock (or if the SA queue is atomic).
> >
> > I wrote a piece of pseudocode about this in my reply, maybe you missed it.
> >
> >>
> >> Let me reiterate the solution we discussed yesterday. After the SA is
> >> disabled
> >> successfully, implementations will stop enqueuing any more events from
> >> that SA
> >> and any call to odp_ipsec_result will indicate that SA is disabled.(SA
> >> Entry is
> >> still valid though). Application may choose to synchronize its database and
> >> process that packet or may drop it. Then it will call odp_ipsec_sa_destroy
> >> which
> >> will delete the SA and any further call to odp_ipsec_result pertaining to
> >> that
> >> SA will result in invalid event. Hope this resolves the issue.
> >
> > That clarifies the API you are proposing but not how you intend it to
> > be used. It is easy to say that an application just should do whatever
> > synchronization is necessary.
> >
> > That said, I think your proposal could be made to work from application
> > perspective, but with some inconvenience:
> >
> > After the packet event handler checks (for every IPsec packet event)
> > that the event is not stale, it has to prevent odp_ipsec_sa_destroy()
> > from being called in any other thread until it no longer uses the SA.
> > While this could be done e.g. using an RCU based or epoch based
> > synchronization scheme to delay the destroy() just long enough, it
> > may perhaps not be easy for all application authors to do so since
> > ODP does not really provide anything than the basic atomics for that.
>
> Wouldn't this happen automatically if the event queue were atomic? Or
> if an ordered lock were used to protect odp_ipsec_result() for the
> duration of the SA context reference? For example:
That approach would work if the event handler destroyed the SA in
the same queue context as it handles the packet events of the SA.
The problem is that SA deletion starts elsewhere in some control thread
and there would have to be a way to reliably delegate the actual destroy
to the correct queue context by having a an event enqueued through the
SA queue. And after the proposed API change the ODP implementation would
not necessarily send such an event (e.g. disable complete event) and
would not allow the application to send any event either.
The approach would work if there are unhandled ipsec packets events
for the SA in the SA queues but that may not be the case. In async case
one could send a dummy packet after disable() to get such an event, but
in the inline case that would not work (well, if one shared the same
event queue between inline and async processing, then that trick could
work).
The atomic queue or ordered lock approach works with the current API
just because there is the disable completion event that can be used
to transfer control to the correct packet processing thread (even if
the actual destroy would be further deferred to some control thread).
A variation of this, with probably significant overhead, would be
such that the ipsec packet event handler just enqueued the events
to an application created normal ODP queue and the application would
generate a suitable "disable complete" event to the same queue and
then the actual event processing in the context of the application
created normal queue could do the needed ordering using an ordered
lock.
So yes, things can be made to work with the proposed API change,
but in a less straightforward manner than with the current API.
Janne
>
> while (1) {
> ev = odp_schedule(&queue, ODP_SCHED_WAIT);
> ev_t = odp_event_types(ev, &ev_sub_t);
>
> switch (ev_sub_t) {
> case ODP_IPSEC_PACKET_IPSEC:
> pkt = odp_ipsec_packet_from_event(ev);
> odp_schedule_order_lock(0); /* Protect SA refs from destroy races */
>
> if (odp_ipsec_result(&ipsec_res, pkt) < 0) { /* Stale event, discard
> */
> odp_schedule_order_unlock(0);
> free_event(ev);
> continue;
> }
>
> if (ipsec_res->status.all != ODP_IPSEC_OK) {
> odp_schedule_order_unlock(0);
> ...Handle IPsec failure
> odp_event_free(ev);
> continue;
> }
>
> ctx = odp_ipsec_sa_context(ipsec_res->sa); /* Get our context
> from result */
>
> ...process the event, referencing SA context as needed
> If we decide to call odp_ipsec_sa_destroy() here, we're OK
> since no other thread can be actively referencing the SA.
>
> odp_schedule_order_unlock(0); /* When we're done with
> referencing the SA */
>
> ...continue event processing without additional SA / ctx referencing.
> break;
>
> case /* Other cases go here */
> ...etc.
> }
> }
>
>
> >
> > The disable completion event is, IMHO, cleaner for ODP API. It is
> > not clear to me if it is impossible to provide it or merely difficult.
> >
> > Janne
> >
