| Message ID | 20170724062415.1542-1-eggi.innovations@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 24, 2017 at 08:24:15AM +0200, Felix Schmoll wrote: > Implement reading from PV console. Making use of polling. > > Signed-off-by: Felix Schmoll <eggi.innovations@gmail.com> > Reviewed-by: Wei Liu <wei.liu2@citrix.com>
On Mon, Jul 24, 2017 at 08:24:15AM +0200, Felix Schmoll wrote: > Implement reading from PV console. Making use of polling. > > Signed-off-by: Felix Schmoll <eggi.innovations@gmail.com> > > --- > Changed since v3: > * Add to comment: read function blocks if no data available > --- > common/console.c | 26 ++++++++++++++++++++++++++ > include/xtf/console.h | 2 ++ > 2 files changed, 28 insertions(+) > > diff --git a/common/console.c b/common/console.c > index 7cb2361..b1a35f4 100644 > --- a/common/console.c > +++ b/common/console.c > @@ -46,6 +46,32 @@ static size_t pv_console_write_some(const char *buf, size_t len) > } > > /* > + * Read out data from the pv ring, either until buffer is filled or no > + * more data are available. Might result in partial strings, depending > + * on how xenconsoled passes in data. > + * > + * Will block if no data are available. > + */ > +size_t pv_console_read_some(char *buf, size_t len) > +{ > + size_t s = 0; > + uint32_t cons, prod; > + > + while ( !test_and_clear_bit(pv_evtchn, shared_info.evtchn_pending) || > + (pv_ring->in_cons == pv_ring->in_prod) ) > + hypercall_poll(pv_evtchn); > + > + cons = pv_ring->in_cons, prod = LOAD_ACQUIRE(&pv_ring->in_prod); Andrew pointed out on IRC this will degrade to cons = prod = LOAD_ACQUIRE(...); I failed to spot this earlier. See http://en.cppreference.com/w/c/language/operator_precedence
On 24/07/17 07:24, Felix Schmoll wrote: > Implement reading from PV console. Making use of polling. > > Signed-off-by: Felix Schmoll <eggi.innovations@gmail.com> > > --- > Changed since v3: > * Add to comment: read function blocks if no data available > --- > common/console.c | 26 ++++++++++++++++++++++++++ > include/xtf/console.h | 2 ++ > 2 files changed, 28 insertions(+) > > diff --git a/common/console.c b/common/console.c > index 7cb2361..b1a35f4 100644 > --- a/common/console.c > +++ b/common/console.c > @@ -46,6 +46,32 @@ static size_t pv_console_write_some(const char *buf, size_t len) > } > > /* > + * Read out data from the pv ring, either until buffer is filled or no > + * more data are available. Might result in partial strings, depending > + * on how xenconsoled passes in data. > + * > + * Will block if no data are available. > + */ > +size_t pv_console_read_some(char *buf, size_t len) > +{ > + size_t s = 0; > + uint32_t cons, prod; > + > + while ( !test_and_clear_bit(pv_evtchn, shared_info.evtchn_pending) || > + (pv_ring->in_cons == pv_ring->in_prod) ) > + hypercall_poll(pv_evtchn); > + > + cons = pv_ring->in_cons, prod = LOAD_ACQUIRE(&pv_ring->in_prod); Given the confusion this has caused on IRC, I'd prefer that the code was explicit. Is the following ok? cons = pv_ring->in_cons; prod = LOAD_ACQUIRE(&pv_ring->in_prod); If so, I can fix up on commit. ~Andrew > + > + while ( (s < len) && (0 < (prod - cons)) ) > + buf[s++] = pv_ring->in[cons++ & (sizeof(pv_ring->in) - 1)]; > + > + STORE_RELEASE(&pv_ring->in_cons, cons); > + > + return s; > +} > + > +/* > * Write some data into the pv ring, synchronously waiting for all data to be > * consumed. > */ > diff --git a/include/xtf/console.h b/include/xtf/console.h > index 2a93c06..caec790 100644 > --- a/include/xtf/console.h > +++ b/include/xtf/console.h > @@ -25,6 +25,8 @@ void init_pv_console(xencons_interface_t *ring, > void vprintk(const char *fmt, va_list args) __printf(1, 0); > void printk(const char *fmt, ...) __printf(1, 2); > > +size_t pv_console_read_some(char *buf, size_t len); > + > #endif /* XTF_CONSOLE_H */ > > /*
2017-07-24 12:38 GMT+02:00 Andrew Cooper <andrew.cooper3@citrix.com>: > > > Given the confusion this has caused on IRC, I'd prefer that the code was > explicit. Is the following ok? > > cons = pv_ring->in_cons; > prod = LOAD_ACQUIRE(&pv_ring->in_prod); > > If so, I can fix up on commit. > > ~Andrew > > I'm not really convinced by this: If this was really happening, then you could never do multiple declarations in the same line because it would always degrade. According to for example Wikipedia, /** * Commas act as separators in this line, not as an operator. * Results: a=1, b=2, c=3, i=0 */int a=1, b=2, c=3, i=0; Also, read_some is obviously modeled after write_some, so you would have to change that one too.
2017-07-25 8:58 GMT+02:00 Felix Schmoll <eggi.innovations@gmail.com>: > 2017-07-24 12:38 GMT+02:00 Andrew Cooper <andrew.cooper3@citrix.com>: >> >> >> Given the confusion this has caused on IRC, I'd prefer that the code was >> explicit. Is the following ok? >> >> cons = pv_ring->in_cons; >> prod = LOAD_ACQUIRE(&pv_ring->in_prod); >> >> If so, I can fix up on commit. >> >> ~Andrew >> >> > I'm not really convinced by this: If this was really happening, then you > could never do multiple declarations in the same line because it would > always degrade. > > According to for example Wikipedia, > > /** * Commas act as separators in this line, not as an operator. * Results: a=1, b=2, c=3, i=0 */int a=1, b=2, c=3, i=0; > > Also, read_some is obviously modeled after write_some, so you would have > to change that one too. > Nevermind, sure, go ahead.
diff --git a/common/console.c b/common/console.c index 7cb2361..b1a35f4 100644 --- a/common/console.c +++ b/common/console.c @@ -46,6 +46,32 @@ static size_t pv_console_write_some(const char *buf, size_t len) } /* + * Read out data from the pv ring, either until buffer is filled or no + * more data are available. Might result in partial strings, depending + * on how xenconsoled passes in data. + * + * Will block if no data are available. + */ +size_t pv_console_read_some(char *buf, size_t len) +{ + size_t s = 0; + uint32_t cons, prod; + + while ( !test_and_clear_bit(pv_evtchn, shared_info.evtchn_pending) || + (pv_ring->in_cons == pv_ring->in_prod) ) + hypercall_poll(pv_evtchn); + + cons = pv_ring->in_cons, prod = LOAD_ACQUIRE(&pv_ring->in_prod); + + while ( (s < len) && (0 < (prod - cons)) ) + buf[s++] = pv_ring->in[cons++ & (sizeof(pv_ring->in) - 1)]; + + STORE_RELEASE(&pv_ring->in_cons, cons); + + return s; +} + +/* * Write some data into the pv ring, synchronously waiting for all data to be * consumed. */ diff --git a/include/xtf/console.h b/include/xtf/console.h index 2a93c06..caec790 100644 --- a/include/xtf/console.h +++ b/include/xtf/console.h @@ -25,6 +25,8 @@ void init_pv_console(xencons_interface_t *ring, void vprintk(const char *fmt, va_list args) __printf(1, 0); void printk(const char *fmt, ...) __printf(1, 2); +size_t pv_console_read_some(char *buf, size_t len); + #endif /* XTF_CONSOLE_H */ /*
Implement reading from PV console. Making use of polling. Signed-off-by: Felix Schmoll <eggi.innovations@gmail.com> --- Changed since v3: * Add to comment: read function blocks if no data available --- common/console.c | 26 ++++++++++++++++++++++++++ include/xtf/console.h | 2 ++ 2 files changed, 28 insertions(+)