On 09/21/2017 02:05 PM, Cornelia Huck wrote:
> On Thu, 21 Sep 2017 13:22:44 +0200
> Halil Pasic <pa...@linux.vnet.ibm.com> wrote:
>
>> On 09/21/2017 11:15 AM, Cornelia Huck wrote:
>>>> +static inline CcwDataStream *get_cds(Terminal3270 *t)
>>>> +{
>>>> + return &(CCW_DEVICE(&t->cdev)->sch->cds);
>>>> +}
>>>> +
>>>> +static int read_payload_3270(EmulatedCcw3270Device *dev)
>>>> {
>>>> Terminal3270 *t = TERMINAL_3270(dev);
>>>> int len;
>>>>
>>>> - len = MIN(count, t->in_len);
>>>> - cpu_physical_memory_write(cda, t->inv, len);
>>>> + len = MIN(ccw_dstream_avail(get_cds(t)), t->in_len);
>>>> + ccw_dstream_write_buf(get_cds(t), t->inv, len);
>>> CCW_DEVICE() as called by get_cds() goes through qom, which implies a
>>> bit of overhead. Not sure if it makes sense to cache it in this
>>> function so you don't go through it multiple times. (Dito for the other
>>> callback.)
>>>
>>
>> I've cargo-culted this way of getting CCW_DEVICE(&t->cdev) to the CcwDevice
>> form terminal_read (the pattern used at multiple places in the file). As
>> far as I can tell, the overhead basically depends on CONFIG_QOM_CAST_DEBUG.
>>
>> I have no idea what do we have in production, but my guess is, that
>> ONFIG_QOM_CAST_DEBUG makes only sense for development and testing
>> (especially if proper test coverage is assumed).
>> Can you enlighten me?
>>
>> CCW_DEVICE() may contain a run-time check (depending on
>> CONFIG_QOM_CAST_DEBUG),
>> we however can make sure things are OK at compile time. This brings
>> me to the next question. Does it even make sense to use OBJECT_CHECK based
>> constructs when going from specific to general (we don't actually need a
>> cast here)? Obviously, for the other direction we really need a cast, so
>> doing
>> a run-time check there does indeed provide added value.
>
> The basic rule seems to be "use a qom cast, unless you are on a fast
> path" - even though qom debug makes the most sense while developing.
>
Does this imply "don't use qom cast on fast path"? I guess that would
mean that we basically expect production builds being with CONFIG_QOM_CAST_DEBUG
defined.
> But let's not turn that into a big discussion: It's probably not really
> worth optimizing here.
>
I agree about the optimization. OTOH I do believe establishing best
practices is important. I'm afraid, I'm seeing much 'more monkey see
monkey do' than healthy (in such an environment prior art is even
more important). I believe, understanding a best practice (candidate)
should always be a part of establishing a best practice.
Sorry, I may be inappropriate (you requested to not turn this into
a big discussion, and I'm afraid this goes in that direction). If
I'm please ignore the stuff above this line.
So, there is nothing to be addressed about about this series so far.
Does this mean good for inclusion once prerequisites are met -- unless
somebody finds something?
Regards,
Halil
