Re: [PATCH 08/10] scsi/scsi_bus: Add scsi_device_get

2020-09-30 Thread Maxim Levitsky 
On Wed, 2020-09-30 at 19:46 +0200, Paolo Bonzini wrote:
> On 30/09/20 16:32, Maxim Levitsky wrote:
> > >   Compared to Maxim's patch, I am avoiding the extra argument
> > >   to do_scsi_device_find by moving the RCU_READ_LOCK_GUARD()
> > >   out of do_scsi_device_find itself.
> > Which is a good idea, although my mindset was like, I got a device,
> > lets just grab a ref to it before it disappears and then do
> > whatever I want.
> 
> Understood, but "I got a device, I know I'm under RCU so it can't
> disappear" is more efficient and just as common.  This also explains the
> difference in patch 7.

Fair point. I am still learing to correctly use RCU.

Best regards,
Maxim Levitsky
> 
> Paolo
>

Re: [PATCH 08/10] scsi/scsi_bus: Add scsi_device_get

2020-09-30 Thread Paolo Bonzini 
On 30/09/20 16:32, Maxim Levitsky wrote:
>>  Compared to Maxim's patch, I am avoiding the extra argument
>>  to do_scsi_device_find by moving the RCU_READ_LOCK_GUARD()
>>  out of do_scsi_device_find itself.
> Which is a good idea, although my mindset was like, I got a device,
> lets just grab a ref to it before it disappears and then do
> whatever I want.

Understood, but "I got a device, I know I'm under RCU so it can't
disappear" is more efficient and just as common.  This also explains the
difference in patch 7.

Paolo

Re: [PATCH 08/10] scsi/scsi_bus: Add scsi_device_get

2020-09-30 Thread Maxim Levitsky 
On Fri, 2020-09-25 at 13:26 -0400, Paolo Bonzini wrote:
> From: Maxim Levitsky 
> 
> Add scsi_device_get which finds the scsi device
> and takes a reference to it.
> 
> Suggested-by: Stefan Hajnoczi 
> Signed-off-by: Maxim Levitsky 
> Message-Id: <20200913160259.32145-8-mlevi...@redhat.com>
> Signed-off-by: Paolo Bonzini 
> ---
>   Compared to Maxim's patch, I am avoiding the extra argument
>   to do_scsi_device_find by moving the RCU_READ_LOCK_GUARD()
>   out of do_scsi_device_find itself.
Which is a good idea, although my mindset was like, I got a device,
lets just grab a ref to it before it disappears and then do
whatever I want. The extra argument was ugly no doubt though.

Best regards,
Maxim Levitsky

> 
>  hw/scsi/scsi-bus.c | 11 +++
>  include/hw/scsi/scsi.h |  1 +
>  2 files changed, 12 insertions(+)
> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 7599113efe..eda8cb7e70 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -73,6 +73,17 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, 
> int id, int lun)
>  return do_scsi_device_find(bus, channel, id, lun, false);
>  }
>  
> +SCSIDevice *scsi_device_get(SCSIBus *bus, int channel, int id, int lun)
> +{
> +SCSIDevice *d;
> +RCU_READ_LOCK_GUARD();
> +d = do_scsi_device_find(bus, channel, id, lun, false);
> +if (d) {
> +object_ref(d);
> +}
> +return d;
> +}
> +
>  static void scsi_device_realize(SCSIDevice *s, Error **errp)
>  {
>  SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(s);
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index 7a55cdbd74..09fa5c9d2a 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -190,6 +190,7 @@ int scsi_device_get_sense(SCSIDevice *dev, uint8_t *buf, 
> int len, bool fixed);
>  int scsi_SG_IO_FROM_DEV(BlockBackend *blk, uint8_t *cmd, uint8_t cmd_size,
>  uint8_t *buf, uint8_t buf_size);
>  SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int target, int lun);
> +SCSIDevice *scsi_device_get(SCSIBus *bus, int channel, int target, int lun);
>  
>  /* scsi-generic.c. */
>  extern const SCSIReqOps scsi_generic_req_ops;

[PATCH 08/10] scsi/scsi_bus: Add scsi_device_get

2020-09-25 Thread Paolo Bonzini 
From: Maxim Levitsky 

Add scsi_device_get which finds the scsi device
and takes a reference to it.

Suggested-by: Stefan Hajnoczi 
Signed-off-by: Maxim Levitsky 
Message-Id: <20200913160259.32145-8-mlevi...@redhat.com>
Signed-off-by: Paolo Bonzini 
---
Compared to Maxim's patch, I am avoiding the extra argument
to do_scsi_device_find by moving the RCU_READ_LOCK_GUARD()
out of do_scsi_device_find itself.

 hw/scsi/scsi-bus.c | 11 +++
 include/hw/scsi/scsi.h |  1 +
 2 files changed, 12 insertions(+)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 7599113efe..eda8cb7e70 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -73,6 +73,17 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int 
id, int lun)
 return do_scsi_device_find(bus, channel, id, lun, false);
 }
 
+SCSIDevice *scsi_device_get(SCSIBus *bus, int channel, int id, int lun)
+{
+SCSIDevice *d;
+RCU_READ_LOCK_GUARD();
+d = do_scsi_device_find(bus, channel, id, lun, false);
+if (d) {
+object_ref(d);
+}
+return d;
+}
+
 static void scsi_device_realize(SCSIDevice *s, Error **errp)
 {
 SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(s);
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 7a55cdbd74..09fa5c9d2a 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -190,6 +190,7 @@ int scsi_device_get_sense(SCSIDevice *dev, uint8_t *buf, 
int len, bool fixed);
 int scsi_SG_IO_FROM_DEV(BlockBackend *blk, uint8_t *cmd, uint8_t cmd_size,
 uint8_t *buf, uint8_t buf_size);
 SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int target, int lun);
+SCSIDevice *scsi_device_get(SCSIBus *bus, int channel, int target, int lun);
 
 /* scsi-generic.c. */
 extern const SCSIReqOps scsi_generic_req_ops;
-- 
2.26.2