Does cli() need to be called before reading avenrun?

Does cli() need to be called before reading avenrun?

[Peter Benie]

In kernel 2.4, in sys_sysinfo(), the code reads:

   cli();
   val.uptime = jiffies / HZ;

   val.loads[0] = avenrun[0] << (SI_LOAD_SHIFT - FSHIFT);
   val.loads[1] = avenrun[1] << (SI_LOAD_SHIFT - FSHIFT);
   val.loads[2] = avenrun[2] << (SI_LOAD_SHIFT - FSHIFT);

   val.procs = nr_threads-1;
   sti();

In loadavg_read_proc, the code is in essence the same, except that it
isn't wrapped in cli/sti.  

Is there a reason for the cli?

Peter

Re: Does cli() need to be called before reading avenrun?

[Robert Love]

On Wed, 2003-01-01 at 19:39, Peter Benie wrote:
> In kernel 2.4, in sys_sysinfo(), the code reads:
> 
>    cli();
>    val.uptime = jiffies / HZ;
> 
>    val.loads[0] = avenrun[0] << (SI_LOAD_SHIFT - FSHIFT);
>    val.loads[1] = avenrun[1] << (SI_LOAD_SHIFT - FSHIFT);
>    val.loads[2] = avenrun[2] << (SI_LOAD_SHIFT - FSHIFT);
> 
>    val.procs = nr_threads-1;
>    sti();
> 
> In loadavg_read_proc, the code is in essence the same, except that it
> isn't wrapped in cli/sti.  
> 
> Is there a reason for the cli?

Interrupts off protects the reading of avenrun[] - it is only written to
from the timer interrupt, so you can safely read it when interrupts are
disabled.

The reason we need some sort of protection is that there are the three
array entries, so we need to make sure we read all three atomically (not
that its a huge deal if we do not).

So I guess the proc code needs to disable interrupts, too.  I am on my
laptop and only have a 2.5 tree - sure it is not disabled somewhere in
there?

Note in 2.5 we replaced the cli() with a read_lock() on xtime_lock.

	Robert Love

Re: Does cli() need to be called before reading avenrun?

[Peter Benie]

Robert Love writes ("Re: Does cli() need to be called before reading avenrun?"):
> On Wed, 2003-01-01 at 19:39, Peter Benie wrote:
> > In kernel 2.4, in sys_sysinfo(), the code reads:
> > 
> >    cli();
> >    val.uptime = jiffies / HZ;
> > 
> >    val.loads[0] = avenrun[0] << (SI_LOAD_SHIFT - FSHIFT);
> >    val.loads[1] = avenrun[1] << (SI_LOAD_SHIFT - FSHIFT);
> >    val.loads[2] = avenrun[2] << (SI_LOAD_SHIFT - FSHIFT);
> > 
> >    val.procs = nr_threads-1;
> >    sti();
> > 
> > In loadavg_read_proc, the code is in essence the same, except that it
> > isn't wrapped in cli/sti.  
> > 
> > Is there a reason for the cli?
> 
> The reason we need some sort of protection is that there are the three
> array entries, so we need to make sure we read all three atomically (not
> that its a huge deal if we do not).

This is exactly what I have my doubts about. These are three load
averages with different time constants - it isn't meaningful to
compare them so it doesn't matter if they are not sampled at exactly
the same time.

What caused me to look at this code is was a complaint in the exim
source about how long sysinfo() took compared with reading
/proc/loadavg - the difference is a factor of 100 which is large
enough to seriously affect exim's performance. This looks like a
possible cause.

Peter