On Wed, Jan 5, 2011 at 9:21 PM, Roland Dreier <rdreier@xxxxxxxxx> wrote:
>
> So, just looking over some of the atomic_t usage here (which is usually
> one of the first places I review):
>
> > +struct srpt_send_ioctx {
> > ...
> > + atomic_t state;
>
> this seems to be accessed only with atomic_read(), atomic_set() and
> atomic_cmpxchg() (without any memory barriers). It's rather hard to see
> that this is correct (and indeed without memory barriers I suspect it's
> not; also since atomic_read() does not protect the state from changing
> immediately afterwards there may be other races).
>
> I think it might be better to handle this in a simpler, more naive
> way -- just have a lock (probably an existing one) that protects the
> contents of state and not use cmpxchg().
>
> In any case since no "real" atomic operations are used, I suspect it
> would be better to just code this in terms of unsigned int and regular
> cmpxchg().
Hello Roland,
Thanks for the feedback.
I'll have a look at converting these atomic operations into regular
locking. The current implementation should be fine though.
Manipulating I/O context state only happens inside callback functions.
These callback functions are either invoked by the SCST core or from
inside an IB completion callback. For each I/O context at most one
such callback function is active at any time. Even if different
callback functions would be invoked on different CPU cores, it is the
responsibility of either the SCST core or the IB core / driver to
provide proper synchronization (memory barrier).
There is one exception though: the function srpt_pending_cmd_timeout()
can in theory proceed concurrently with an IB completion callback.
This is why I/O context state manipulation was implemented via atomic
operations. That race is unlikely to cause any harm though: the
callback srpt_pending_cmd_timeout() is only invoked if 60 seconds
(srpt_template.max_hw_pending_time) after an IB send was posted no IB
completion has been received. This timeout should be a magnitude
larger than the time needed to receive an IB completion indicating
either success or failure.
Please note that with IB firmware that complies to the IB specs the
function srpt_pending_cmd_timeout() will never be invoked because
before that function gets invoked an IB completion will already have
been received.
> 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 ?
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.
Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
