Re: [PATCH 4/7] [SCSI] scst: Add SRP target driver

[Roland Dreier <rdreier@cisco.com>]

 > I'll have a look at converting these atomic operations into regular
 > locking. The current implementation should be fine though.

I believe that the current implementation is correct.  However it is
much harder for someone naive like me to understand, since using cmpxchg
is much subtler than just using a lock to protect data.  If this isn't
on the hottest of hot paths then I think cmpxchg is over-optimization --
much better to have less fancy code.

 > > Also, there is processing_compl:
 > >
 > >  > +static void srpt_completion(struct ib_cq *cq, void *ctx)
 > >  > +{
 > >  > +    struct srpt_rdma_ch *ch = ctx;
 > >  > +
 > >  > +    BUG_ON(!ch);
 > >  > +    atomic_inc(&ch->processing_compl);
 > >
 > > and
 > >
 > >  > +static void srpt_unregister_channel(struct srpt_rdma_ch *ch)
 > >  > ...
 > >  > +    while (atomic_read(&ch->processing_compl))
 > >  > +            ;
 > >
 > > this seems racy to me -- I don't see any reason why we couldn't have:
 > >
 > >        srpt_completion()
 > >
 > >                                        srpt_unregister_channel()
 > >                                          processing_compl == 0,
 > >                                          continue
 > >
 > >          atomic_inc(&ch->processing_compl);
 > >
 > >                                          finish unregistering channel
 > >
 > >          use unregistered channel
 > 
 > I'm not sure what the above pseudo-code should do ?

I'm trying to illustrate the race by showing what two different contexts
might be doing -- the left column is one context, the right column is
the other context.

In this specific case, if the while loop testing processing_compl
actually ever does anything then I don't see how it could be safe -- if
srpt_completion() could be executing during the while loop, then I don't
see anything that prevents the while loop from finishing before
srpt_completion does its atomic_inc().

 > Regarding the current implementation: there is a hard requirement in
 > SCST that no new commands are queued for a given session after
 > scst_unregister_session() has been invoked. So
 > scst_unregister_session() must only be invoked after the IB queue pair
 > has been reset *and* srpt_completion() has finished. It would be great
 > if that could be implemented without using one or another kind of
 > counter. I'm not sure however whether it is possible to eliminate the
 > "processing_compl" counter entirely.

I don't pretend to understand the flow of code here.  But it seems the
only safe way to implement this cleanup code is to stop posting work
requests and then wait until all the requests you've posted are
complete.  Trying to wait for the completion handler to stop executing
does not seem possible to implement in a safe way.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html