On Fri, 8 Sep 2017, Nikhil Agarwal wrote:
> On 7 September 2017 at 14:09, Peltonen, Janne (Nokia - FI/Espoo)
> <[email protected]> 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.
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
