Messages in this thread | | | Date | Mon, 28 Sep 2020 14:29:34 +0800 | From | Shuo A Liu <> | Subject | Re: [PATCH v4 06/17] virt: acrn: Introduce VM management interfaces |
| |
On Mon 28.Sep'20 at 7:25:16 +0200, Greg Kroah-Hartman wrote: >On Mon, Sep 28, 2020 at 11:50:30AM +0800, Shuo A Liu wrote: >> > > + write_lock_bh(&acrn_vm_list_lock); >> > > + list_add(&vm->list, &acrn_vm_list); >> > > + write_unlock_bh(&acrn_vm_list_lock); >> > >> > Why are the _bh() variants being used here? >> > >> > You are only accessing this list from userspace context in this patch. >> > >> > Heck, you aren't even reading from the list, only writing to it... >> >> acrn_vm_list is read in a tasklet which dispatch I/O requests and is wrote >> in VM creation ioctl. Use the rwlock mechanism to protect it. >> The reading operation is introduced in the following patches of this >> series. So i keep the lock type at the moment of introduction. > >Ok, but think about someone trying to review this code. Does this lock >actually make sense here? No, it does not. How am I supposed to know >to look at future patches to determine that it changes location and >usage to require this?
OK. May i know how to handle such kind of code submission? Or which way following do you prefer? 1) Use a mutex lock here, then change it to rwlock in a later patch of this series. 2) Add more comments in changelog about the lock. (Now, there is comment around the acrn_vm_list_lock)
> >That's just not fair, would you want to review something like this? > >And a HUGE meta-comment, again, why am I the only one reviewing this >stuff? Why do you have a ton of Intel people on the Cc: yet it is, once >again, my job to do this?
The patchset has been reviewed in Intel's internal mailist several rounds and got Reviewed-by: before send out. That's why i Cced many Intel people as well.
This patchset is all about a common driver for the ACRN hypervisor support. I put the code in drivers/virt/ and found you are one of the maintainer of vboxguest driver which is in the same subdirectory. I thought you should be the right person to be Cced when i submitted this series.
Certainly, any comments are welcome. And really appreciate your review and help. I have little experience to submit a new driver to the community, my apologies if thing goes wrong.
Thanks shuo
| |