[Qemu-devel] [PATCH] SCSI bus: fix to incomplete QOMify

[Qemu-devel] [PATCH] SCSI bus: fix to incomplete QOMify

[Cao jin]

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/scsi/scsi-bus.c     | 16 ++++++++--------
 include/hw/scsi/scsi.h |  5 -----
 2 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index fea0257..1667e01 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -54,7 +54,7 @@ static void scsi_device_realize(SCSIDevice *s, Error **errp)
 int scsi_bus_parse_cdb(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf,
                        void *hba_private)
 {
-    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
+    SCSIBus *bus = SCSI_BUS(dev->qdev.parent_bus);
     int rc;
 
     assert(cmd->len == 0);
@@ -145,7 +145,7 @@ static void scsi_dma_restart_cb(void *opaque, int running, RunState state)
 static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
 {
     SCSIDevice *dev = SCSI_DEVICE(qdev);
-    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
+    SCSIBus *bus = SCSI_BUS(dev->qdev.parent_bus);
     SCSIDevice *d;
     Error *local_err = NULL;
 
@@ -553,7 +553,7 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
                             uint32_t tag, uint32_t lun, void *hba_private)
 {
     SCSIRequest *req;
-    SCSIBus *bus = scsi_bus_from_device(d);
+    SCSIBus *bus = SCSI_BUS(d->qdev.parent_bus);
     BusState *qbus = BUS(bus);
     const int memset_off = offsetof(SCSIRequest, sense)
                            + sizeof(req->sense);
@@ -578,7 +578,7 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
 SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
                           uint8_t *buf, void *hba_private)
 {
-    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, d->qdev.parent_bus);
+    SCSIBus *bus = SCSI_BUS(d->qdev.parent_bus);
     const SCSIReqOps *ops;
     SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(d);
     SCSIRequest *req;
@@ -1272,7 +1272,7 @@ int scsi_req_parse_cdb(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf)
 
 void scsi_device_report_change(SCSIDevice *dev, SCSISense sense)
 {
-    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
+    SCSIBus *bus = SCSI_BUS(dev->qdev.parent_bus);
 
     scsi_device_set_ua(dev, sense);
     if (bus->info->change) {
@@ -1612,7 +1612,7 @@ void scsi_req_unref(SCSIRequest *req)
     assert(req->refcount > 0);
     if (--req->refcount == 0) {
         BusState *qbus = req->dev->qdev.parent_bus;
-        SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, qbus);
+        SCSIBus *bus = SCSI_BUS(qbus);
 
         if (bus->info->free_request && req->hba_private) {
             bus->info->free_request(bus, req->hba_private);
@@ -1896,7 +1896,7 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
 static void put_scsi_requests(QEMUFile *f, void *pv, size_t size)
 {
     SCSIDevice *s = pv;
-    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, s->qdev.parent_bus);
+    SCSIBus *bus = SCSI_BUS(s->qdev.parent_bus);
     SCSIRequest *req;
 
     QTAILQ_FOREACH(req, &s->requests, next) {
@@ -1921,7 +1921,7 @@ static void put_scsi_requests(QEMUFile *f, void *pv, size_t size)
 static int get_scsi_requests(QEMUFile *f, void *pv, size_t size)
 {
     SCSIDevice *s = pv;
-    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, s->qdev.parent_bus);
+    SCSIBus *bus = SCSI_BUS(s->qdev.parent_bus);
     int8_t sbyte;
 
     while ((sbyte = qemu_get_sbyte(f)) > 0) {
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 1915a73..2ca1d7b 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -167,11 +167,6 @@ struct SCSIBus {
 void scsi_bus_new(SCSIBus *bus, size_t bus_size, DeviceState *host,
                   const SCSIBusInfo *info, const char *bus_name);
 
-static inline SCSIBus *scsi_bus_from_device(SCSIDevice *d)
-{
-    return DO_UPCAST(SCSIBus, qbus, d->qdev.parent_bus);
-}
-
 SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockBackend *blk,
                                       int unit, bool removable, int bootindex,
                                       const char *serial, Error **errp);
-- 
2.1.0

Re: [Qemu-devel] [PATCH] SCSI bus: fix to incomplete QOMify

[Paolo Bonzini]

On 06/01/2016 14:43, Cao jin wrote:
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/scsi/scsi-bus.c     | 16 ++++++++--------
>  include/hw/scsi/scsi.h |  5 -----
>  2 files changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index fea0257..1667e01 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -54,7 +54,7 @@ static void scsi_device_realize(SCSIDevice *s, Error **errp)
>  int scsi_bus_parse_cdb(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf,
>                         void *hba_private)
>  {
> -    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
> +    SCSIBus *bus = SCSI_BUS(dev->qdev.parent_bus);
>      int rc;
>  
>      assert(cmd->len == 0);
> @@ -145,7 +145,7 @@ static void scsi_dma_restart_cb(void *opaque, int running, RunState state)
>  static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
>  {
>      SCSIDevice *dev = SCSI_DEVICE(qdev);
> -    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
> +    SCSIBus *bus = SCSI_BUS(dev->qdev.parent_bus);
>      SCSIDevice *d;
>      Error *local_err = NULL;
>  
> @@ -553,7 +553,7 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
>                              uint32_t tag, uint32_t lun, void *hba_private)
>  {
>      SCSIRequest *req;
> -    SCSIBus *bus = scsi_bus_from_device(d);
> +    SCSIBus *bus = SCSI_BUS(d->qdev.parent_bus);
>      BusState *qbus = BUS(bus);
>      const int memset_off = offsetof(SCSIRequest, sense)
>                             + sizeof(req->sense);
> @@ -578,7 +578,7 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
>  SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
>                            uint8_t *buf, void *hba_private)
>  {
> -    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, d->qdev.parent_bus);
> +    SCSIBus *bus = SCSI_BUS(d->qdev.parent_bus);
>      const SCSIReqOps *ops;
>      SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(d);
>      SCSIRequest *req;
> @@ -1272,7 +1272,7 @@ int scsi_req_parse_cdb(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf)
>  
>  void scsi_device_report_change(SCSIDevice *dev, SCSISense sense)
>  {
> -    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
> +    SCSIBus *bus = SCSI_BUS(dev->qdev.parent_bus);
>  
>      scsi_device_set_ua(dev, sense);
>      if (bus->info->change) {
> @@ -1612,7 +1612,7 @@ void scsi_req_unref(SCSIRequest *req)
>      assert(req->refcount > 0);
>      if (--req->refcount == 0) {
>          BusState *qbus = req->dev->qdev.parent_bus;
> -        SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, qbus);
> +        SCSIBus *bus = SCSI_BUS(qbus);
>  
>          if (bus->info->free_request && req->hba_private) {
>              bus->info->free_request(bus, req->hba_private);
> @@ -1896,7 +1896,7 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
>  static void put_scsi_requests(QEMUFile *f, void *pv, size_t size)
>  {
>      SCSIDevice *s = pv;
> -    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, s->qdev.parent_bus);
> +    SCSIBus *bus = SCSI_BUS(s->qdev.parent_bus);
>      SCSIRequest *req;
>  
>      QTAILQ_FOREACH(req, &s->requests, next) {
> @@ -1921,7 +1921,7 @@ static void put_scsi_requests(QEMUFile *f, void *pv, size_t size)
>  static int get_scsi_requests(QEMUFile *f, void *pv, size_t size)
>  {
>      SCSIDevice *s = pv;
> -    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, s->qdev.parent_bus);
> +    SCSIBus *bus = SCSI_BUS(s->qdev.parent_bus);
>      int8_t sbyte;
>  
>      while ((sbyte = qemu_get_sbyte(f)) > 0) {
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index 1915a73..2ca1d7b 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -167,11 +167,6 @@ struct SCSIBus {
>  void scsi_bus_new(SCSIBus *bus, size_t bus_size, DeviceState *host,
>                    const SCSIBusInfo *info, const char *bus_name);
>  
> -static inline SCSIBus *scsi_bus_from_device(SCSIDevice *d)
> -{
> -    return DO_UPCAST(SCSIBus, qbus, d->qdev.parent_bus);
> -}
> -
>  SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockBackend *blk,
>                                        int unit, bool removable, int bootindex,
>                                        const char *serial, Error **errp);
> 

These functions are called in the data path; changes to use SCSI_BUS()
should come with performance data proving that it doesn't slow down I/O.

Paolo

Re: [Qemu-devel] [PATCH] SCSI bus: fix to incomplete QOMify

[Cao jin]

On 01/07/2016 05:38 PM, Paolo Bonzini wrote:
>
>
> On 06/01/2016 14:43, Cao jin wrote:
[...]
>
> These functions are called in the data path; changes to use SCSI_BUS()
> should come with performance data proving that it doesn't slow down I/O.
>
> Paolo
>

I see. I am not familiar with the procedure of scsi i/o performance 
test, do have some guideline about it?

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH] SCSI bus: fix to incomplete QOMify

[Paolo Bonzini]

On 07/01/2016 10:53, Cao jin wrote:
> On 01/07/2016 05:38 PM, Paolo Bonzini wrote:
>> On 06/01/2016 14:43, Cao jin wrote:
>> These functions are called in the data path; changes to use SCSI_BUS()
>> should come with performance data proving that it doesn't slow down I/O.
>>
> 
> I see. I am not familiar with the procedure of scsi i/o performance
> test, do have some guideline about it?

Usually people test with FIO, but I think it's simpler to just keep
DO_UPCAST here.

Paolo

Re: [Qemu-devel] [PATCH] SCSI bus: fix to incomplete QOMify

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 07/01/2016 10:53, Cao jin wrote:
>> On 01/07/2016 05:38 PM, Paolo Bonzini wrote:
>>> On 06/01/2016 14:43, Cao jin wrote:
>>> These functions are called in the data path; changes to use SCSI_BUS()
>>> should come with performance data proving that it doesn't slow down I/O.
>>>
>> 
>> I see. I am not familiar with the procedure of scsi i/o performance
>> test, do have some guideline about it?
>
> Usually people test with FIO, but I think it's simpler to just keep
> DO_UPCAST here.

DO_UPCAST() needs to die.

SCSI_BUS() is a readable wrapper around OBJECT_CHECK():

    #define SCSI_BUS(obj) OBJECT_CHECK(SCSIBus, (obj), TYPE_SCSI_BUS)

OBJECT_CHECK() is semantically a type cast, but in actual code it does
more:

    #define OBJECT_CHECK(type, obj, name) \
        ((type *)object_dynamic_cast_assert(OBJECT(obj), (name), \
                                            __FILE__, __LINE__, __func__))

    Object *object_dynamic_cast_assert(Object *obj, const char *typename,
                                       const char *file, int line, const char *func)
    {
        trace_object_dynamic_cast_assert(obj ? obj->class->type->name : "(null)",
                                         typename, file, line, func);

    #ifdef CONFIG_QOM_CAST_DEBUG
    [...]
    #endif
        return obj;
    }

If CONFIG_QOM_CAST_DEBUG is on, it checks.  That's a feature.

If CONFIG_QOM_CAST_DEBUG is off, it still calls to trace.  That might
be a misfeature.

If OBJECT_CHECK() isn't fit for fast paths because of that, perhaps QOM
should provide a conversion macro that is.  Andreas, what do you think?

Re: [Qemu-devel] [PATCH] SCSI bus: fix to incomplete QOMify

[Peter Maydell]

On 11 January 2016 at 17:37, Markus Armbruster <armbru@redhat.com> wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> On 07/01/2016 10:53, Cao jin wrote:
>>> On 01/07/2016 05:38 PM, Paolo Bonzini wrote:
>>>> On 06/01/2016 14:43, Cao jin wrote:
>>>> These functions are called in the data path; changes to use SCSI_BUS()
>>>> should come with performance data proving that it doesn't slow down I/O.
>>>>
>>>
>>> I see. I am not familiar with the procedure of scsi i/o performance
>>> test, do have some guideline about it?
>>
>> Usually people test with FIO, but I think it's simpler to just keep
>> DO_UPCAST here.
>
> DO_UPCAST() needs to die.
>
> SCSI_BUS() is a readable wrapper around OBJECT_CHECK():
>
>     #define SCSI_BUS(obj) OBJECT_CHECK(SCSIBus, (obj), TYPE_SCSI_BUS)
>
> OBJECT_CHECK() is semantically a type cast, but in actual code it does
> more:

Yes. We need to stop avoiding our QOM cast macros because of
performance worries. (We already do this for the CPU casts.)
We either need to fix the QOM cast macros to be faster, or
accept the performance hit.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] SCSI bus: fix to incomplete QOMify

[Andreas Färber]

Am 11.01.2016 um 18:37 schrieb Markus Armbruster:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>> On 07/01/2016 10:53, Cao jin wrote:
>>> On 01/07/2016 05:38 PM, Paolo Bonzini wrote:
>>>> On 06/01/2016 14:43, Cao jin wrote:
>>>> These functions are called in the data path; changes to use SCSI_BUS()
>>>> should come with performance data proving that it doesn't slow down I/O.
>>>
>>> I see. I am not familiar with the procedure of scsi i/o performance
>>> test, do have some guideline about it?
>>
>> Usually people test with FIO, but I think it's simpler to just keep
>> DO_UPCAST here.
> 
> DO_UPCAST() needs to die.
> 
> SCSI_BUS() is a readable wrapper around OBJECT_CHECK():
> 
>     #define SCSI_BUS(obj) OBJECT_CHECK(SCSIBus, (obj), TYPE_SCSI_BUS)
> 
> OBJECT_CHECK() is semantically a type cast, but in actual code it does
> more:
> 
>     #define OBJECT_CHECK(type, obj, name) \
>         ((type *)object_dynamic_cast_assert(OBJECT(obj), (name), \
>                                             __FILE__, __LINE__, __func__))
> 
>     Object *object_dynamic_cast_assert(Object *obj, const char *typename,
>                                        const char *file, int line, const char *func)
>     {
>         trace_object_dynamic_cast_assert(obj ? obj->class->type->name : "(null)",
>                                          typename, file, line, func);
> 
>     #ifdef CONFIG_QOM_CAST_DEBUG
>     [...]
>     #endif
>         return obj;
>     }
> 
> If CONFIG_QOM_CAST_DEBUG is on, it checks.  That's a feature.
> 
> If CONFIG_QOM_CAST_DEBUG is off, it still calls to trace.  That might
> be a misfeature.

Ugh, I was not aware of that tracing!

Also, might it make sense to make that ..._assert() function inline?

> If OBJECT_CHECK() isn't fit for fast paths because of that, perhaps QOM
> should provide a conversion macro that is.  Andreas, what do you think?

We had a similar issue with virtio fast paths. I'm not sure whether we
kept DO_UPCAST() or used a direct (Foo *) cast though. I think mst was a
fan of upcast, and I used the latter in TCG innards.

The general idea (from Anthony) was that derived types should not know
about the implementation detail that the struct has a field of a certain
name (for example, a switch to C++ classes or something would make it go
away). So there's two issues mixed here, a) going from foo(x,y,z) to
just bar(x) and b) the type checking as part of bar(x).

If you want to propose a FOO_FAST() or whatever, I'm all ears.

Cheers,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)