public inbox for linux-rt-users@vger.kernel.org
 help / color / mirror / Atom feed
From: Nicholas Mc Guire <der.herr@hofr.at>
To: linux-rt-users@vger.kernel.org
Cc: LKML <linux-kernel@vger.kernel.org>,
	Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
	Steven Rostedt <rostedt@goodmis.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Carsten Emde <C.Emde@osadl.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Andreas Platschek <platschek@ict.tuwien.ac.at>
Subject: [PATCH RT 2/5] allow preemption in alloc and free _buffer_head
Date: Mon, 10 Feb 2014 16:37:59 +0100	[thread overview]
Message-ID: <20140210153759.GC20017@opentech.at> (raw)


allow preemption in alloc and free _buffer_head

fs/buffer.c:3339
void free_buffer_head(struct buffer_head *bh)
{
        BUG_ON(!list_empty(&bh->b_assoc_buffers));
        kmem_cache_free(bh_cachep, bh);
        preempt_disable();
        __this_cpu_dec(bh_accounting.nr);
        recalc_bh_state();
        preempt_enable();
}
migrate_disable here should do

Interleavings that could occure if preemption is allowed:

1) Simple interleaving: 
  T1 dec
           T2 dec
           T2 recalc
  T1 recalc

that would not be a problem the second recalc would not change anything it
would only set buffer_heads_over_limit again which is idempotent. If the
first recalc hit the ratelimit so will the second and equally if the first
did not hit the ratelimit.

2a) premption induced race in __this_cpu_dec
  T1 dec:start
               T2 dec
  T1 dec:end

__this_cpu_dec 
  -> __this_cpu_sub 
    -> __this_cpu_add
      -> __this_cpu_add_#
        -> __this_cpu_generic_to_op
         -> __this_cpu_ptr += val


so we end up with the possibilities of
  T1 load
          T2 load
          T2 store
  T1 store
and the increment provided by T2 would be lost. The result of which
is that reaching the buffer_heads_over_limit threshold in recalc_bh_state 
static void recalc_bh_state(void)
{
        int i;
        int tot = 0;

        if (__this_cpu_inc_return(bh_accounting.ratelimit) - 1 < 4096)
                return;
        __this_cpu_write(bh_accounting.ratelimit, 0);
        for_each_online_cpu(i)
                tot += per_cpu(bh_accounting, i).nr;
        buffer_heads_over_limit = (tot > max_buffer_heads);
}
would be reached a bit later - but given that this is a one-line race, and 
thus low probability - if it does happen it would not significantly impact 
reaching the buffer_heads_over_limit - as this is not a precise process
anyway (ratelimited) it should not have any functional impact.

2b) preemption induced race in recalc_bh_state
  T1 recalc:start
               T2 recalc
  T1 recalc:end
This case is not really a race as the only write operations are
to a local variable and ultimately to the buffer_heads_over_limit. The only
thing that could happen is that the buffer_heads_over_limit would remain
set if T1s recalc used and older, thus higher, bh_accounting.nr and sets the
buffer_heads_over_limit to 1 even though the second recalc had intermediately 
set it to 0. This would be fixed at the next recalc_bh_state execution.

The argument for alloc_buffer_head is along the same line - the potential
side-effects due to low-probability races seem negligable.

patch against 3.12.10-rt15

Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
---
 fs/buffer.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 01eaa17..e2e4dd7 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -3327,10 +3327,10 @@ struct buffer_head *alloc_buffer_head(gfp_t gfp_flags)
 	if (ret) {
 		INIT_LIST_HEAD(&ret->b_assoc_buffers);
 		buffer_head_init_locks(ret);
-		preempt_disable();
+		migrate_disable();
 		__this_cpu_inc(bh_accounting.nr);
 		recalc_bh_state();
-		preempt_enable();
+		migrate_enable();
 	}
 	return ret;
 }
@@ -3340,10 +3340,10 @@ void free_buffer_head(struct buffer_head *bh)
 {
 	BUG_ON(!list_empty(&bh->b_assoc_buffers));
 	kmem_cache_free(bh_cachep, bh);
-	preempt_disable();
+	migrate_disable();
 	__this_cpu_dec(bh_accounting.nr);
 	recalc_bh_state();
-	preempt_enable();
+	migrate_enable();
 }
 EXPORT_SYMBOL(free_buffer_head);
 
-- 
1.7.2.5


             reply	other threads:[~2014-02-10 15:38 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-02-10 15:37 Nicholas Mc Guire [this message]
2014-02-14 12:53 ` [PATCH RT 2/5] allow preemption in alloc and free _buffer_head Sebastian Andrzej Siewior

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140210153759.GC20017@opentech.at \
    --to=der.herr@hofr.at \
    --cc=C.Emde@osadl.org \
    --cc=bigeasy@linutronix.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rt-users@vger.kernel.org \
    --cc=peterz@infradead.org \
    --cc=platschek@ict.tuwien.ac.at \
    --cc=rostedt@goodmis.org \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox