Re: [PATCH 3/4] relayfs for 2.6.10: locking/lockless implementation

[Robert Wisniewski <bob@watson.ibm.com>]

Greg KH writes:
 > 
 > A: Because it messes up the order in which people normally read text.
 > Q: Why is top-posting such a bad thing?

Okay - sorry about that - will respond inline.


 > A: Top-posting.
 > Q: What is the most annoying thing in e-mail?

ACTUALLY I HATE ALL CAPS MORE :-)

 > A: No.
 > Q: Should I include quotations after my reply?
 > 
 > On Fri, Jan 14, 2005 at 05:57:21PM -0500, Robert Wisniewski wrote:
 > > Greg,
 > >      There are a couple variables used throughout relayfs code that could
 > > be modified at any point "simultaneously" by different processes.  These
 > > variables were not declared volatile, thus when we modify them we need to
 > > tell the compiler to refetch from memory as another process could have
 > > changed out from under the current stream of execution since the last time
 > > there were accessed in the function.  An alternative would be to mark the
 > > variables that we care about as volatile.
 > 
 > marking them volatile does not protect across cpus.  Just using a normal
 > atomic_t will work properly.

I believe the below illustrates the problem that will be seen without
volatile.

int g;

funct1()
{
	g = 0;
	while (g == 0) ;
	printf("here\n");
}

funct2()
{
	g = 1;
}


If funct1 and funct2 are executed either by different processes on the same
processor (which is the case for relayfs as we have per-processor buffers),
or across processors, and if g is not marked volatile, then if a process
gets into funct1, does the first line, gets interrupted, funct2 executes,
then funct1 continues, funct1 will never get out of the while loop.  If
however g is marked volatile then it will get out of the loop.  Is this not
true?  If we agree that in this case g needs to be marked volatile there
are now two choices.  Either mark it volatile in the declaration or
while(g==0) barrier(); If I understand it correctly barrier() invalidates
all current register values and forces a reload from memory.

 > > I am not sure how best to make
 > > that tradeoff (i.e., always forcing a refetch by marking a variable
 > > volatile or only at points were we know we need to by memory clobbering) or
 > > on what side the Linux community comes down on.  We certainly would be
 > > happy to go either way with the relayfs code, i.e., mark them variable and
 > > used the standard atomic operations.
 > 
 > Just use atomic_t and don't mess with volatile.  See the archives for
 > why that (volatile) doesn't work like that.
 > 
 > > That explains compare_and_store, atomic_add, and atomic_sub.
 > 
 > No it doesn't, why do your own version of this function with the
 > barrier() function?
 > 
 > > It does not explain the memory clobbering around the atomic set
 > > operation, which I'm guessing was there just to be consistent with the
 > > other operations, and could, I believe, be removed.  Hopefully that
 > > helps answer the question.  If it doesn't please feel free to ask
 > > more.  Thanks.
 > 
 > So these can just be removed, and the code changed to use the proper
 > atomic calls?  If so, please do so.

Yes we can remove the code and use the standard atomic calls, but based on
the above example, I think we need to mark a couple variables volatile.  Do
you agree, if so, and unless there's dissenting opinion we can make the
change.

Thanks.

-bob

 > 
 > thanks,
 > 
 > greg k-h