Re: [Xen-devel] [PATCH v2 3/5] dbg_rw_guest_mem: Conditionally enable debug log output

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-01-08 at 19:10 +0100, Tim Deegan wrote:
> At 17:44 +0000 on 08 Jan (1389199462), Ian Campbell wrote:
> > On Wed, 2014-01-08 at 18:04 +0100, Tim Deegan wrote:
> > > At 16:47 +0000 on 08 Jan (1389196026), Ian Campbell wrote:
> > > > On Wed, 2014-01-08 at 09:28 -0500, Don Slutz wrote:
> > > > > > Using volatile is almost always wrong. Why do you think it is needed
> > > > > > here?
> > > > > 
> > > > > This was from Mukesh Rathor:
> > > > > 
> > > > > http://lists.xen.org/archives/html/xen-devel/2014-01/msg00426.html
> > > > > 
> > > > > I saw no reason to make it volatile, but maybe "kdb" needs this? Happy
> > > > > to change any way you want.
> > > > 
> > > > I'm not the maintainer but if I were I'd say drop the volatile and maybe
> > > > mark it __read_mostly and perhaps __used too (since it's static it might
> > > > otherwise get eliminated).
> > > > 
> > > > > > If anything this variable is exactly the opposite, i..e 
> > > > > > __read_mostly or
> > > > > > even const (given that I can't see anything which writes it I 
> > > > > > suppose
> > > > > > this is a compile time setting?)
> > > > > 
> > > > > That has been how I have been testing it so far (changing the source
> > > > > to set values).  Mukesh claims to be able to change it at will.  Not
> > > > > sure how const may effect this.
> > > 
> > > If the idea is to use kdb itself to frob the value, then it does need
> > > something to stop the compiler caching it.  This might even be one of
> > > the few cases where 'volatile' actually DTRT; it would still be more
> > > in keeping with Xen style to use an explicit read op (like
> > > atomic_read()) where the value is consumed.
> > 
> > Is there any need to be asynchronously frobbing this value in the middle
> > of a function within this file and expecting it to be reliable? I'd have
> > thought that changing the value and having it take affect on the next
> > debug event/hypercall/whatever would be what was wanted.
> 
> The variable is static and there's nothing in the file that updates
> it, so the compiler might drop it entirely.  Maybe __used__ would be
> good enough to stop the compiler dropping all reads, but I'm not sure.

Isn't that exactly what __used (or __used__) is for?

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel