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

[Tim Deegan <tim@xxxxxxx>]

At 08:41 +0000 on 09 Jan (1389253311), Ian Campbell wrote:
> 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?

The docs say that "the variable must be emitted even if it appears
that the variable is not referenced" but what that means for
individual accesses I don't know.  So, probably?  I would be inclined
to distruct the compiler here and just wrap the accesses in a
read_atomic.

Tim.

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