Re: [BUG] Something goes wrong with timer statistics.

[Björn Steinbrink <B.Steinbrink@gmx.de>]

On 2007.06.01 00:59:11 +0200, Ian Kumlien wrote:
> On tor, 2007-05-31 at 17:27 +0200, Björn Steinbrink wrote:
> > On 2007.05.31 17:10:07 +0200, Eric Dumazet wrote:
> > > Well... :) , there is still a memory barrier missing it seems.
> > > 
> > > Another cpu might see a bad value if 'active=1' is set before tstat_hash_table is really cleared.
> > 
> > Hm, that even makes the assumption in my first try valid ;-)
> > Just for the record, this time I thought that the barrier from the
> > spinlock in timer_stats_update_stats (right before the check for active)
> > would be enough, but that's obviously running on the wrong cpu if we
> > race... *sigh*
> > 
> 
> Fix the comment below and you can add:
> Signed-off-by: Ian Kumlien <pomac@vapor.com>
> 
> It's currently been running for the longest period ever, ie, 11 minutes
> =)

Finally! :-)

> I'm gonna leave it running during the night and send a status update
> when the evil daystar reaches it's peak CET. (i haven't been able to
> stop since linus mentioned it... damn it... =))
> 
> > @@ -360,6 +364,7 @@ static ssize_t tstats_write(struct file *file, const char __user *buf,
> >  		if (!active) {
> >  			reset_entries();
> >  			time_start = ktime_get();
> > +			smb_mb();
> 
> smb? you mean smp =)

No idea how I managed to break that after test compiling it... :-/
Thanks for catching it :-)

So unless I messed up something again, here's the final patch.

Thanks,
Björn


Fix two races in the timer stats lookup code. One by ensuring that the
initialization of a new entry is finished upon insertion of that entry.
The other by cleaning up the hash table when the entries array is
cleared, so that we don't have any "pre-inserted" entries.

Thanks to Eric Dumazet for reminding me of the memory barriers.

Signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>
Signed-off-by: Ian Kumlien <pomac@vapor.com>
---
diff --git a/kernel/time/timer_stats.c b/kernel/time/timer_stats.c
index 868f1bc..fa3d380 100644
--- a/kernel/time/timer_stats.c
+++ b/kernel/time/timer_stats.c
@@ -117,21 +117,6 @@ static struct entry entries[MAX_ENTRIES];
 
 static atomic_t overflow_count;
 
-static void reset_entries(void)
-{
-	nr_entries = 0;
-	memset(entries, 0, sizeof(entries));
-	atomic_set(&overflow_count, 0);
-}
-
-static struct entry *alloc_entry(void)
-{
-	if (nr_entries >= MAX_ENTRIES)
-		return NULL;
-
-	return entries + nr_entries++;
-}
-
 /*
  * The entries are in a hash-table, for fast lookup:
  */
@@ -149,6 +134,22 @@ static struct entry *alloc_entry(void)
 
 static struct entry *tstat_hash_table[TSTAT_HASH_SIZE] __read_mostly;
 
+static void reset_entries(void)
+{
+	nr_entries = 0;
+	memset(entries, 0, sizeof(entries));
+	memset(tstat_hash_table, 0, sizeof(tstat_hash_table));
+	atomic_set(&overflow_count, 0);
+}
+
+static struct entry *alloc_entry(void)
+{
+	if (nr_entries >= MAX_ENTRIES)
+		return NULL;
+
+	return entries + nr_entries++;
+}
+
 static int match_entries(struct entry *entry1, struct entry *entry2)
 {
 	return entry1->timer       == entry2->timer	  &&
@@ -202,12 +203,15 @@ static struct entry *tstat_lookup(struct entry *entry, char *comm)
 	if (curr) {
 		*curr = *entry;
 		curr->count = 0;
+		curr->next = NULL;
 		memcpy(curr->comm, comm, TASK_COMM_LEN);
+
+		smp_mb(); /* Ensure that curr is initialized before insert */
+
 		if (prev)
 			prev->next = curr;
 		else
 			*head = curr;
-		curr->next = NULL;
 	}
  out_unlock:
 	spin_unlock(&table_lock);
@@ -360,6 +364,7 @@ static ssize_t tstats_write(struct file *file, const char __user *buf,
 		if (!active) {
 			reset_entries();
 			time_start = ktime_get();
+			smp_mb();
 			active = 1;
 		}
 		break;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/