| Message ID | 20170920172314.102710-2-pasic@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | 3270 improvements | expand |
On Wed, 20 Sep 2017 19:23:13 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > Let us convert the 3270 code so it uses the recently introduced > CcwDataStream abstraction instead of blindly assuming direct data access. > > This patch does not change behavior beyond introducing IDA support: for > direct data access CCWs everything stays as-is. (If there are bugs, they > are also preserved). > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> > Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> > --- > hw/char/terminal3270.c | 18 +++++++++++------- > hw/s390x/3270-ccw.c | 4 ++-- > include/hw/s390x/3270-ccw.h | 5 ++--- > 3 files changed, 15 insertions(+), 12 deletions(-) > > diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c > index 28f599111d..c976a63cc2 100644 > --- a/hw/char/terminal3270.c > +++ b/hw/char/terminal3270.c > @@ -182,14 +182,18 @@ static void terminal_init(EmulatedCcw3270Device *dev, Error **errp) > terminal_read, chr_event, NULL, t, NULL, true); > } > > -static int read_payload_3270(EmulatedCcw3270Device *dev, uint32_t cda, > - uint16_t count) > +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.) > t->in_len -= len; > > return len; Looks good.
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. Halil >> t->in_len -= len; >> >> return len;
On Thu, 21 Sep 2017 13:22:44 +0200 Halil Pasic <pasic@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. But let's not turn that into a big discussion: It's probably not really worth optimizing here.
On 09/21/2017 02:05 PM, Cornelia Huck wrote: > On Thu, 21 Sep 2017 13:22:44 +0200 > Halil Pasic <pasic@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
On Thu, 21 Sep 2017 18:11:06 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > 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? It is in my pipeline, and I currently don't see anything wrong with it. More reviews are still welcome, though, as always :)
diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c index 28f599111d..c976a63cc2 100644 --- a/hw/char/terminal3270.c +++ b/hw/char/terminal3270.c @@ -182,14 +182,18 @@ static void terminal_init(EmulatedCcw3270Device *dev, Error **errp) terminal_read, chr_event, NULL, t, NULL, true); } -static int read_payload_3270(EmulatedCcw3270Device *dev, uint32_t cda, - uint16_t count) +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); t->in_len -= len; return len; @@ -222,11 +226,11 @@ static int insert_IAC_escape_char(uint8_t *outv, int out_len) * Write 3270 outbound to socket. * Return the count of 3270 data field if succeeded, zero if failed. */ -static int write_payload_3270(EmulatedCcw3270Device *dev, uint8_t cmd, - uint32_t cda, uint16_t count) +static int write_payload_3270(EmulatedCcw3270Device *dev, uint8_t cmd) { Terminal3270 *t = TERMINAL_3270(dev); int retval = 0; + int count = ccw_dstream_avail(get_cds(t)); assert(count <= (OUTPUT_BUFFER_SIZE - 3) / 2); @@ -244,7 +248,7 @@ static int write_payload_3270(EmulatedCcw3270Device *dev, uint8_t cmd, return count; } t->outv[0] = cmd; - cpu_physical_memory_read(cda, &t->outv[1], count); + ccw_dstream_read_buf(get_cds(t), &t->outv[1], count); t->out_len = count + 1; t->out_len = insert_IAC_escape_char(t->outv, t->out_len); diff --git a/hw/s390x/3270-ccw.c b/hw/s390x/3270-ccw.c index 1554aa2484..eaca28e224 100644 --- a/hw/s390x/3270-ccw.c +++ b/hw/s390x/3270-ccw.c @@ -28,7 +28,7 @@ static int handle_payload_3270_read(EmulatedCcw3270Device *dev, CCW1 *ccw) return -EFAULT; } - len = ck->read_payload_3270(dev, ccw->cda, ccw->count); + len = ck->read_payload_3270(dev); ccw_dev->sch->curr_status.scsw.count = ccw->count - len; return 0; @@ -45,7 +45,7 @@ static int handle_payload_3270_write(EmulatedCcw3270Device *dev, CCW1 *ccw) return -EFAULT; } - len = ck->write_payload_3270(dev, ccw->cmd_code, ccw->cda, ccw->count); + len = ck->write_payload_3270(dev, ccw->cmd_code); if (len <= 0) { return -EIO; diff --git a/include/hw/s390x/3270-ccw.h b/include/hw/s390x/3270-ccw.h index 46bee2533c..9d1d18e2bd 100644 --- a/include/hw/s390x/3270-ccw.h +++ b/include/hw/s390x/3270-ccw.h @@ -45,9 +45,8 @@ typedef struct EmulatedCcw3270Class { CCWDeviceClass parent_class; void (*init)(EmulatedCcw3270Device *, Error **); - int (*read_payload_3270)(EmulatedCcw3270Device *, uint32_t, uint16_t); - int (*write_payload_3270)(EmulatedCcw3270Device *, uint8_t, uint32_t, - uint16_t); + int (*read_payload_3270)(EmulatedCcw3270Device *); + int (*write_payload_3270)(EmulatedCcw3270Device *, uint8_t); } EmulatedCcw3270Class; #endif