| Message ID | 1495033046-16427-1-git-send-email-anshul.makkar@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 17.05.17 at 16:57, <anshul.makkar@citrix.com> wrote: > The condition to check for if there is space in the ring buffer > also becomes true if the buffer is full, thus consumer waits for > the producer to fill the buffer eventhough it is already full. > > To resolve the situation, check if the buffer is full and then > break from the loop. > e.g case: prod = 1272, cons = 248. > > Signed-off-by: Anshul Makkar <anshul.makkar@citrix.com> Please avoid indenting the entire commit message. > --- a/tools/firmware/hvmloader/xenbus.c > +++ b/tools/firmware/hvmloader/xenbus.c > @@ -141,7 +141,18 @@ static void ring_read(char *data, uint32_t len) > /* Don't overrun the producer pointer */ > while ( (part = MASK_XENSTORE_IDX(rings->rsp_prod - > rings->rsp_cons)) == 0 ) > + { > + /* don't wait for producer to fill the ring if it is already full. > + * Condition happens when you write string > 1K into the ring. > + * eg case prod=1272 cons=248. > + */ Comment style. > + if ( !(XENSTORE_RING_SIZE - (rings->rsp_prod - rings->rsp_cons)) ) Is this any different from if ( rings->rsp_prod - rings->rsp_cons == XENSTORE_RING_SIZE ) ? Jan
On 17/05/2017 16:56, Jan Beulich wrote: >>>> On 17.05.17 at 16:57, <anshul.makkar@citrix.com> wrote: >> The condition to check for if there is space in the ring buffer >> also becomes true if the buffer is full, thus consumer waits for >> the producer to fill the buffer eventhough it is already full. >> >> To resolve the situation, check if the buffer is full and then >> break from the loop. >> e.g case: prod = 1272, cons = 248. >> >> Signed-off-by: Anshul Makkar <anshul.makkar@citrix.com> > > Please avoid indenting the entire commit message. Oh. sorry about that. I saw this format in few previous commits and adopted it. Will correct it. > >> --- a/tools/firmware/hvmloader/xenbus.c >> +++ b/tools/firmware/hvmloader/xenbus.c >> @@ -141,7 +141,18 @@ static void ring_read(char *data, uint32_t len) >> /* Don't overrun the producer pointer */ >> while ( (part = MASK_XENSTORE_IDX(rings->rsp_prod - >> rings->rsp_cons)) == 0 ) >> + { >> + /* don't wait for producer to fill the ring if it is already full. >> + * Condition happens when you write string > 1K into the ring. >> + * eg case prod=1272 cons=248. >> + */ > > Comment style. > >> + if ( !(XENSTORE_RING_SIZE - (rings->rsp_prod - rings->rsp_cons)) ) > > Is this any different from > > if ( rings->rsp_prod - rings->rsp_cons == XENSTORE_RING_SIZE ) No, its same. Ok, will use the suggested approach. > > ? > > Jan >
Jan Beulich writes ("Re: [Xen-devel] [PATCH] XenBus: Don't wait for producer to fill the ring if the ring"):
> On 17.05.17 at 16:57, <anshul.makkar@citrix.com> wrote:
> > --- a/tools/firmware/hvmloader/xenbus.c
> > +++ b/tools/firmware/hvmloader/xenbus.c
> > @@ -141,7 +141,18 @@ static void ring_read(char *data, uint32_t len)
> > /* Don't overrun the producer pointer */
> > while ( (part = MASK_XENSTORE_IDX(rings->rsp_prod -
> > rings->rsp_cons)) == 0 )
> > + {
> > + /* don't wait for producer to fill the ring if it is already full.
> > + * Condition happens when you write string > 1K into the ring.
> > + * eg case prod=1272 cons=248.
> > + */
>
> Comment style.
What specifically are you complaining about here ?
If you're talking about the fact that the opening /* is on the same
line as the first line of text, observe that the rest of
hvmloader/xenbus.c has most multi-line comments in these styles:
/* If the backend reads the state while we're erasing it then the
* ring state will become corrupted, preventing guest frontends from
* connecting. This is rare. To help diagnose the failure, we fill
* the ring with XS_INVALID packets. */
/* Read a xenstore key. Returns a nul-terminated string (even if the XS
* data wasn't nul-terminated) or NULL. The returned string is in a
* static buffer, so only valid until the next xenstore/xenbus operation.
* If @default_resp is specified, it is returned in preference to a NULL or
* empty string received from xenstore.
*/
The only exceptions are the copyright notice and the local variables
block.
So if this is what you are complaining about, I think this is
unhelpful nit-picking, given the state of rest of the hvmloader source
code. It obviously doesn't really matter whether /* and */ are on a
line by themselves.
At the very least, if you're going to insist on some particular style,
you could:
* acknowledge that the existing style is not always consistent
and that therefore the submitter couldn't necessarily be expected
to get this "right" (whatever that means) without help
* clearly state what your problem is and which style is to be
used (instead of the obvious candidate which is the style
of the surrounding code) rather than just writing `comment style'
Or maybe you are talking about the lack of a capital d in "don't" ?
I agree that a capital letter would be better. But I think "comment
style" is quite an opaque way of making that observation.
Or perhaps you are talking about some other perceived problem with
this, but of course the reader of your message needs to try to guess
from what you wrote.
Thanks, and sorry to tetch.
Ian.
>>> On 23.05.17 at 14:42, <ian.jackson@eu.citrix.com> wrote: > Jan Beulich writes ("Re: [Xen-devel] [PATCH] XenBus: Don't wait for producer > to fill the ring if the ring"): >> On 17.05.17 at 16:57, <anshul.makkar@citrix.com> wrote: >> > --- a/tools/firmware/hvmloader/xenbus.c >> > +++ b/tools/firmware/hvmloader/xenbus.c >> > @@ -141,7 +141,18 @@ static void ring_read(char *data, uint32_t len) >> > /* Don't overrun the producer pointer */ >> > while ( (part = MASK_XENSTORE_IDX(rings->rsp_prod - >> > rings->rsp_cons)) == 0 ) >> > + { >> > + /* don't wait for producer to fill the ring if it is already full. >> > + * Condition happens when you write string > 1K into the ring. >> > + * eg case prod=1272 cons=248. >> > + */ >> >> Comment style. > > What specifically are you complaining about here ? > > If you're talking about the fact that the opening /* is on the same > line as the first line of text, observe that the rest of > hvmloader/xenbus.c has most multi-line comments in these styles: > > /* If the backend reads the state while we're erasing it then the > * ring state will become corrupted, preventing guest frontends from > * connecting. This is rare. To help diagnose the failure, we fill > * the ring with XS_INVALID packets. */ > > /* Read a xenstore key. Returns a nul-terminated string (even if the XS > * data wasn't nul-terminated) or NULL. The returned string is in a > * static buffer, so only valid until the next xenstore/xenbus operation. > * If @default_resp is specified, it is returned in preference to a NULL or > * empty string received from xenstore. > */ > > The only exceptions are the copyright notice and the local variables > block. > > So if this is what you are complaining about, I think this is > unhelpful nit-picking, given the state of rest of the hvmloader source > code. It obviously doesn't really matter whether /* and */ are on a > line by themselves. > > At the very least, if you're going to insist on some particular style, > you could: > > * acknowledge that the existing style is not always consistent > and that therefore the submitter couldn't necessarily be expected > to get this "right" (whatever that means) without help > > * clearly state what your problem is and which style is to be > used (instead of the obvious candidate which is the style > of the surrounding code) rather than just writing `comment style' I admit that in the given case I didn't go look in what state the file as a whole is. Yet while indeed I _could_ do better with my comment, I even more so think that submitters _could_ do better by not introducing obvious style violations. The more I as a reviewer see such, the more I'm inclined to use very terse comments. I realize this may occasionally be unfair, as the particular contributor may not have had much other history. But please do realize that with the amount of patches these days being _far_ beyond what I can reasonably review (and apparently others too, or I'd have seen quite a few more reviews recently), it doesn't look very reasonable for me to always go and write extended explanations of what's wrong (and no, I don't fancy simply skipping such cosmetic items - when they're really easy to correct I sometimes offer fixing them while committing). I try to do so when I recognize newcomers. > Or maybe you are talking about the lack of a capital d in "don't" ? > I agree that a capital letter would be better. But I think "comment > style" is quite an opaque way of making that observation. And indeed I've given the comment because of both of the issues you name. > Thanks, and sorry to tetch. While I can vaguely guess what you mean here, would you mind helping out with neither me nor my dictionary knowing the word "tetch"? Jan
Jan Beulich writes ("Re: [Xen-devel] [PATCH] XenBus: Don't wait for producer to fill the ring if the ring"):
> On 23.05.17 at 14:42, <ian.jackson@eu.citrix.com> wrote:
> > Thanks, and sorry to tetch.
[ much deleted because I really don't see much point me me arguing
this further ]
> While I can vaguely guess what you mean here, would you mind
> helping out with neither me nor my dictionary knowing the word
> "tetch"?
`To tetch' is the verb from `tetchy', which means something like
`irritable'.
Ian.
diff --git a/tools/firmware/hvmloader/xenbus.c b/tools/firmware/hvmloader/xenbus.c index 448157d..f8fd730 100644 --- a/tools/firmware/hvmloader/xenbus.c +++ b/tools/firmware/hvmloader/xenbus.c @@ -141,7 +141,18 @@ static void ring_read(char *data, uint32_t len) /* Don't overrun the producer pointer */ while ( (part = MASK_XENSTORE_IDX(rings->rsp_prod - rings->rsp_cons)) == 0 ) + { + /* don't wait for producer to fill the ring if it is already full. + * Condition happens when you write string > 1K into the ring. + * eg case prod=1272 cons=248. + */ + if ( !(XENSTORE_RING_SIZE - (rings->rsp_prod - rings->rsp_cons)) ) + { + part = XENSTORE_RING_SIZE; + break; + } ring_wait(); + } /* Don't overrun the end of the ring */ if ( part > (XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(rings->rsp_cons)) ) part = XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(rings->rsp_cons);
The condition to check for if there is space in the ring buffer also becomes true if the buffer is full, thus consumer waits for the producer to fill the buffer eventhough it is already full. To resolve the situation, check if the buffer is full and then break from the loop. e.g case: prod = 1272, cons = 248. Signed-off-by: Anshul Makkar <anshul.makkar@citrix.com> --- tools/firmware/hvmloader/xenbus.c | 11 +++++++++++ 1 file changed, 11 insertions(+)