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

["Andreas Färber" <afaerber@suse.de>]

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)