| Message ID | 1452087782-980-1-git-send-email-caoj.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
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
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?
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
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?
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
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
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);
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(-)