lkml.org 
[lkml]   [2012]   [Dec]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 5/5] virtio-scsi: introduce multiqueue support
Il 18/12/2012 16:03, Michael S. Tsirkin ha scritto:
> On Tue, Dec 18, 2012 at 03:08:08PM +0100, Paolo Bonzini wrote:
>> Il 18/12/2012 14:57, Michael S. Tsirkin ha scritto:
>>>> -static int virtscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
>>>> +static int virtscsi_queuecommand(struct virtio_scsi *vscsi,
>>>> + struct virtio_scsi_target_state *tgt,
>>>> + struct scsi_cmnd *sc)
>>>> {
>>>> - struct virtio_scsi *vscsi = shost_priv(sh);
>>>> - struct virtio_scsi_target_state *tgt = &vscsi->tgt[sc->device->id];
>>>> struct virtio_scsi_cmd *cmd;
>>>> + struct virtio_scsi_vq *req_vq;
>>>> int ret;
>>>>
>>>> struct Scsi_Host *shost = virtio_scsi_host(vscsi->vdev);
>>>> @@ -461,7 +533,8 @@ static int virtscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
>>>> BUG_ON(sc->cmd_len > VIRTIO_SCSI_CDB_SIZE);
>>>> memcpy(cmd->req.cmd.cdb, sc->cmnd, sc->cmd_len);
>>>>
>>>> - if (virtscsi_kick_cmd(tgt, &vscsi->req_vq, cmd,
>>>> + req_vq = ACCESS_ONCE(tgt->req_vq);
>>>
>>> This ACCESS_ONCE without a barrier looks strange to me.
>>> Can req_vq change? Needs a comment.
>>
>> Barriers are needed to order two things. Here I don't have the second thing
>> to order against, hence no barrier.
>>
>> Accessing req_vq lockless is safe, and there's a comment about it, but you
>> still want ACCESS_ONCE to ensure the compiler doesn't play tricks.
>
> That's just it.
> Why don't you want compiler to play tricks?

Because I want the lockless access to occur exactly when I write it.
Otherwise I have one more thing to think about, i.e. what a crazy
compiler writer could do with my code. And having been on the other
side of the trench, compiler writers can have *really* crazy ideas.

Anyhow, I'll reorganize the code to move the ACCESS_ONCE closer to the
write and make it clearer.

>>>> + if (virtscsi_kick_cmd(tgt, req_vq, cmd,
>>>> sizeof cmd->req.cmd, sizeof cmd->resp.cmd,
>>>> GFP_ATOMIC) == 0)
>>>> ret = 0;
>>>> @@ -472,6 +545,48 @@ out:
>>>> return ret;
>>>> }
>>>>
>>>> +static int virtscsi_queuecommand_single(struct Scsi_Host *sh,
>>>> + struct scsi_cmnd *sc)
>>>> +{
>>>> + struct virtio_scsi *vscsi = shost_priv(sh);
>>>> + struct virtio_scsi_target_state *tgt = &vscsi->tgt[sc->device->id];
>>>> +
>>>> + atomic_inc(&tgt->reqs);
>>>
>>> And here we don't have barrier after atomic? Why? Needs a comment.
>>
>> Because we don't write req_vq, so there's no two writes to order. Barrier
>> against what?
>
> Between atomic update and command. Once you queue command it
> can complete and decrement reqs, if this happens before
> increment reqs can become negative even.

This is not a problem. Please read Documentation/memory-barrier.txt:

The following also do _not_ imply memory barriers, and so may
require explicit memory barriers under some circumstances
(smp_mb__before_atomic_dec() for instance):

atomic_add();
atomic_sub();
atomic_inc();
atomic_dec();

If they're used for statistics generation, then they probably don't
need memory barriers, unless there's a coupling between statistical
data.

This is the single-queue case, so it falls under this case.

>>>> /* Discover virtqueues and write information to configuration. */
>>>> - err = vdev->config->find_vqs(vdev, 3, vqs, callbacks, names);
>>>> + err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names);
>>>> if (err)
>>>> return err;
>>>>
>>>> - virtscsi_init_vq(&vscsi->ctrl_vq, vqs[0]);
>>>> - virtscsi_init_vq(&vscsi->event_vq, vqs[1]);
>>>> - virtscsi_init_vq(&vscsi->req_vq, vqs[2]);
>>>> + virtscsi_init_vq(&vscsi->ctrl_vq, vqs[0], false);
>>>> + virtscsi_init_vq(&vscsi->event_vq, vqs[1], false);
>>>> + for (i = VIRTIO_SCSI_VQ_BASE; i < num_vqs; i++)
>>>> + virtscsi_init_vq(&vscsi->req_vqs[i - VIRTIO_SCSI_VQ_BASE],
>>>> + vqs[i], vscsi->num_queues > 1);
>>>
>>> So affinity is true if >1 vq? I am guessing this is not
>>> going to do the right thing unless you have at least
>>> as many vqs as CPUs.
>>
>> Yes, and then you're not setting up the thing correctly.
>
> Why not just check instead of doing the wrong thing?

The right thing could be to set the affinity with a stride, e.g. CPUs
0-4 for virtqueue 0 and so on until CPUs 3-7 for virtqueue 3.

Paolo

>> Isn't the same thing true for virtio-net mq?
>>
>> Paolo
>
> Last I looked it checked vi->max_queue_pairs == num_online_cpus().
> This is even too aggressive I think, max_queue_pairs >=
> num_online_cpus() should be enough.
>



\
 
 \ /
  Last update: 2012-12-18 17:21    [W:0.086 / U:0.072 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site