[Xen-devel] Minor synchronisation quibble in scsifront

[Steven Smith <steven.smith@xxxxxxxxxx>]

I've been having a look through scsifront again, and I saw this bit:

        ring_req->timeout_per_command = (sc->timeout_per_command / HZ);
        ring_req->nr_segments         = 0;

        spin_unlock_irq(host->host_lock);
        scsifront_do_request(info);     
        wait_event_interruptible(info->shadow[ring_req->rqid].wq_reset,
                         info->shadow[ring_req->rqid].wait_reset);

in scsifront_dev_reset_handler().  Looking at scsifront_do_request():

static void scsifront_do_request(struct vscsifrnt_info *info)
{
        struct vscsiif_front_ring *ring = &(info->ring);
        unsigned int irq = info->irq;
        int notify;

        RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(ring, notify);
        if (notify)
                notify_remote_via_irq(irq);
}

scsifront_do_request() is also called from scsifront_queuecommand(),
where it's under the host lock.  I can't see any other relevant
synchronisation, so I think that you might be able to end up with
several processors pushing requests at the same time.  I'm not sure
what'll happen then, but I doubt it's a good idea.

The issue could be avoided if you just swapped two lines in
scsifront_dev_reset_handler():

        ring_req->timeout_per_command = (sc->timeout_per_command / HZ);
        ring_req->nr_segments         = 0;

        scsifront_do_request(info);     
        spin_unlock_irq(host->host_lock);
        wait_event_interruptible(info->shadow[ring_req->rqid].wq_reset,
                         info->shadow[ring_req->rqid].wait_reset);

Does that sound sane?

On the plus side, that's the only strange bit I could see in current
scsifront. :)

Steven.

Attachment: signature.asc
Description: Digital signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel