Re: [Qemu-devel] [PATCH 1/2] s390x/3270: IDA support for 3270 via CcwDataStream

[Cornelia Huck]

On Thu, 21 Sep 2017 13:22:44 +0200
Halil Pasic <address@hidden> 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.

But let's not turn that into a big discussion: It's probably not really
worth optimizing here.