mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: "Radim Krčmář" <rkrcmar@redhat.com>
To: linux-kernel@vger.kernel.org
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Subject: [PATCH 0/7] static_key: fix timer bugs & change api (a bit)
Date: Thu, 17 Oct 2013 12:10:23 +0200	[thread overview]
Message-ID: <1382004631-25895-1-git-send-email-rkrcmar@redhat.com> (raw)

While fixing a simple crash on kvm module unload

1: rate_limit timer can fire after we free its key's module memory.

I noticed other deficiencies in jump_label/static_key code
(The most important is 4.)

2: jump_label_rate_limit() uses an initializator and thus cannot be used
   more than once, leaving no good way to change the timeout.

I have made the API easier on programmers: [1/7]
 * timer is automatically flushed on rmmod
 * jump_label_rate_limit() initializes only once
    - pretty hacky, but we cannot automatically initialize on
      kernel_init/insmod due to insufficient information and while I
      would love getting it through another section, it is probably
      better to do a useless check with this low-rate operation
    * we could flush the timer on change
    * 'init()' + 'set()' (+ 'exit()' ?) would be an alternative ...

3: schedule_delayed_work() does not queue the work if one is already
   pending, but atomic_inc(&key->enabled) is called anyway in
   static_key_slow_dec_deferred(), with the false belief it will be
   decreased when the timer fires.

Fixed in [2,3,4/7], by addition of static_key_slow_inc_deferred().

I'm still not happy with the final design: we don't want delayed
decrease, we just don't want change more that once a interval.
A good solution should immediately enable/disable if interval since last
change has already passed.
I'll generalize ratelimit (probably) to suit our needs, but it won't be
quick, so we could use this in the meantime.

4: static_key_{true,false}() is a horrible name for a representation of
   a boolean and its use is unnecessarily restricted

In patch [5/7], static keys are transformed so one key can be hinted
both ways. This is done by bloating the jump_entry.

Patch [6/7] changes the name to static_key_{likely,unlikely}() because
we no longer have incompatible behaviour.

5: jump_label_init() is called too late

I've seen some patches that debugged the case where we use static_keys
before patching.
Moving jump_label_init() near the top of start_kernel() should help us
avoid it. [7/7]

n: jump_label and static_key api should be split;
   static_key_deferred isn't complete api, or subclass of static_key;
   some functions should be renamed, some removed;
   ...

There are already some patches prepared, but the diffstat isn't pretty,
so I'm keeping them to ripen.

Applied on top of torvalds-3.12-rc5.

Radim Krčmář (7):
  static_key: flush rate limit timer on rmmod
  static_key: add static_key_slow_inc_deferred()
  static_key: keep deferred enabled counter debt
  static_key: use static_key_slow_inc_deferred()
  jump_label: relax branch hinting restrictions
  static_key: use {,un}likely instead of {tru,fals}e
  init: execute jump_label_init() earlier

 Documentation/static-keys.txt         | 39 ++++++++------------
 arch/arm/include/asm/jump_label.h     | 19 +++++++---
 arch/arm/kernel/jump_label.c          |  2 +-
 arch/mips/include/asm/jump_label.h    | 19 +++++++---
 arch/mips/kernel/jump_label.c         |  2 +-
 arch/powerpc/include/asm/jump_label.h | 19 +++++++---
 arch/powerpc/kernel/jump_label.c      |  2 +-
 arch/s390/include/asm/jump_label.h    | 19 +++++++---
 arch/s390/kernel/jump_label.c         |  2 +-
 arch/sparc/include/asm/jump_label.h   | 19 +++++++---
 arch/sparc/kernel/jump_label.c        |  2 +-
 arch/x86/include/asm/jump_label.h     | 19 +++++++---
 arch/x86/include/asm/spinlock.h       |  4 +--
 arch/x86/kernel/jump_label.c          | 32 +++--------------
 arch/x86/kvm/lapic.c                  |  7 ++--
 arch/x86/kvm/lapic.h                  |  6 ++--
 arch/x86/kvm/mmu_audit.c              |  2 +-
 include/linux/context_tracking.h      | 10 +++---
 include/linux/jump_label.h            | 66 +++++++++++++---------------------
 include/linux/jump_label_ratelimit.h  |  6 ++++
 include/linux/memcontrol.h            |  2 +-
 include/linux/netfilter.h             |  2 +-
 include/linux/perf_event.h            |  6 ++--
 include/linux/tick.h                  |  2 +-
 include/linux/tracepoint.h            |  4 +--
 include/linux/vtime.h                 |  2 +-
 include/net/sock.h                    |  4 +--
 init/main.c                           |  6 +++-
 kernel/context_tracking.c             |  4 +--
 kernel/events/core.c                  |  6 ++--
 kernel/jump_label.c                   | 68 ++++++++++++++++++++---------------
 kernel/sched/core.c                   |  4 ++-
 kernel/sched/cputime.c                |  2 +-
 kernel/sched/fair.c                   |  2 +-
 kernel/sched/sched.h                  |  4 +--
 lib/crc-t10dif.c                      |  2 +-
 net/core/dev.c                        |  8 ++---
 net/ipv4/udp.c                        |  4 +--
 net/ipv6/udp.c                        |  4 +--
 39 files changed, 231 insertions(+), 201 deletions(-)

-- 
1.8.3.1

             reply	other threads:[~2013-10-17 10:17 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-10-17 10:10 Radim Krčmář [this message]
2013-10-17 10:10 ` [PATCH 1/7] static_key: flush rate limit timer on rmmod Radim Krčmář
2013-10-17 10:35   ` Paolo Bonzini
2013-10-18  7:24     ` Radim Krčmář
2013-10-17 10:10 ` [PATCH 2/7] static_key: add static_key_slow_inc_deferred() Radim Krčmář
2013-10-17 10:10 ` [PATCH 3/7] static_key: keep deferred enabled counter debt Radim Krčmář
2013-10-17 10:10 ` [PATCH 4/7] static_key: use static_key_slow_inc_deferred() Radim Krčmář
2013-10-17 10:39   ` Paolo Bonzini
2013-10-18  7:29     ` Radim Krčmář
2013-10-17 10:10 ` [PATCH 5/7] jump_label: relax branch hinting restrictions Radim Krčmář
2013-10-17 17:35   ` Steven Rostedt
2013-10-18  7:30     ` Radim Krčmář
2013-10-17 10:10 ` [PATCH 7/7] init: execute jump_label_init() earlier Radim Krčmář
2013-10-17 10:43 ` [PATCH 0/7] static_key: fix timer bugs & change api (a bit) Paolo Bonzini

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=1382004631-25895-1-git-send-email-rkrcmar@redhat.com \
    --to=rkrcmar@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    /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

all inboxes | Powered by JetHome®