On 8 September 2017 at 06:10, Janne Peltonen <[email protected]> wrote: > > > 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). > This would mean, all IPSec processing would have to happen using ordered queues even though there is no need. For ex: in the encryption and look-aside case, the packets do not need to be processed in-order.
> 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. > RCU is a mechanism which needs to be tweaked to the actual application that uses it. I do not think ODP needs to provide any support for RCU based approach. This is secondary, can be discussed further. But the method you have described above is a good solution. I think the IPSec case we are trying to solve can be generalized as how to synchronize deletion of state between dataplane and control plane cores. For ex: take the case of route deletion, there needs to be some synchronization mechanism between control plane core and data plane cores to make sure the data plane cores do not seg-fault when the route is deleted. i.e. synchronization issues do exist in other parts (that do not need ODP or do not use any accelerators) of the application. So, the application already has mechanisms to deal with the issues and application can apply them to IPSec as well. > 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 >
