Asias He <asias@xxxxxxxxxx> writes:
> On Mon, Apr 22, 2013 at 04:17:04PM +0300, Michael S. Tsirkin wrote:
>> > > > > > > > + evt = kzalloc(sizeof(*evt), GFP_KERNEL);
>> > > > > > >
>> > > > > > > I think kzalloc not needed here, you init all fields.
>> > > > > >
>> > > > > > Not really! evt->event.lun[4-7] is not initialized. It needs to be 0.
>> > > > >
>> > > > > So that is 4 bytes just init them when you set rest of lun.
>> > > >
>> > > > It is not in the fast path. You can do it this way but not a must.
>> > >
>> > > I think that's a bit cleaner than relying on kzalloc to zero-initialize.
>> >
>> > I think it is a bit shorter, 4 lines saved.
>>
>> OTOH you don't have to think "what about the rest of the bytes?" then
>> hunt through the code and go "ah, it's kzalloc".
>> Looks it's a style issue but I prefer explicit initialization.
>
> It is just make none sense to argue about this. If you really want you
> can make patches to remove all the lines where use kzalloc because it is
> cleaner.
I prefer explicit initialization too, FWIW.
>> > > > > > > > +static void tcm_vhost_do_evt_work(struct vhost_scsi *vs,
>> > > > > > > > + struct tcm_vhost_evt *evt)
>> > > > > > > > +{
>> > > > > > > > + struct vhost_virtqueue *vq = &vs->vqs[VHOST_SCSI_VQ_EVT];
>> > > > > > > > + struct virtio_scsi_event *event = &evt->event;
>> > > > > > > > + struct virtio_scsi_event __user *eventp;
>> > > > > > > > + unsigned out, in;
>> > > > > > > > + int head, ret;
>> > > > > > > > +
>> > > > > > > > + if (!tcm_vhost_check_endpoint(vq))
>> > > > > > > > + return;
>> > > > > > > > +
>> > > > > > > > + mutex_lock(&vs->vs_events_lock);
>> > > > > > > > + mutex_lock(&vq->mutex);
>> > > > > > > > +again:
>> > > > > > > > + vhost_disable_notify(&vs->dev, vq);
>> > > > > > > > + head = vhost_get_vq_desc(&vs->dev, vq, vq->iov,
>> > > > > > > > + ARRAY_SIZE(vq->iov), &out, &in,
>> > > > > > > > + NULL, NULL);
>> > > > > > > > + if (head < 0) {
>> > > > > > > > + vs->vs_events_dropped = true;
>> > > > > > > > + goto out;
>> > > > > > > > + }
>> > > > > > > > + if (head == vq->num) {
>> > > > > > > > + if (vhost_enable_notify(&vs->dev, vq))
>> > > > > > > > + goto again;
>> > > > > > >
>> > > > > > > Please write loops with while() or for().
>> > > > > > > Not with goto. goto is for error handling.
>> > > > > >
>> > > > > > This makes extra indention which is more ugly.
>> > > > >
>> > > > > I don't care. No loops with goto and that's a hard rule.
>> > > >
>> > > > It is not a loop.
>> > >
>> > > If same code repeats, it's a loop.
>> >
>> > We really need here is to call vhost_get_vq_desc once. It's not a loop
>> > like other places to use while() or for() with vhost_get_vq_desc.
>>
>> Exactly. Like a mutex for example? We only need to
>> lock it once. See __mutex_lock_common - uses a for loop,
>> doesn't it?
>
> So what?
>
>> So fix this please, use while or for or even until.
>
> I do not think it is necessary.
I tend to use again: in such corner cases, where I don't expect it to
be invoked very often. A do loop looks too normal.
I do exactly the same thing in virtio-net.c, for the same case.
Would you like me to review the entire thing?
Cheers,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
