Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Linux: Kernel

[PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction

 

 

First page Previous page 1 2 Next page Last page  View All Linux kernel RSS feed   Index | Next | Previous | View Threaded


jeremy at goop

May 30, 2008, 5:04 PM

Post #1 of 26 (584 views)
Permalink
[PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction

This patch adds an API for doing read-modify-write updates to a pte's
protection bits which may race against hardware updates to the pte.
After reading the pte, the hardware may asynchonously set the accessed
or dirty bits on a pte, which would be lost when writing back the
modified pte value.

The existing technique to handle this race is to use
ptep_get_and_clear() atomically fetch the old pte value and clear it
in memory. This has the effect of marking the pte as non-present,
which will prevent the hardware from updating its state. When the new
value is written back, the pte will be present again, and the hardware
can resume updating the access/dirty flags.

When running in a virtualized environment, pagetable updates are
relatively expensive, since they generally involve some trap into the
hypervisor. To mitigate the cost of these updates, we tend to batch
them.

However, because of the atomic nature of ptep_get_and_clear(), it is
inherently non-batchable. This new interface allows batching by
giving the underlying implementation enough information to open a
transaction between the read and write phases:

ptep_modify_prot_start() returns the current pte value, and puts the
pte entry into a state where either the hardware will not update the
pte, or if it does, the updates will be preserved on commit.

ptep_modify_prot_commit() writes back the updated pte, makes sure that
any hardware updates made since ptep_modify_prot_start() are
preserved.

ptep_modify_prot_start() and _commit() must be exactly paired, and
used while holding the appropriate pte lock. They do not protect
against other software updates of the pte in any way.

The current implementations of ptep_modify_prot_start and _commit are
functionally unchanged from before: _start() uses ptep_get_and_clear()
fetch the pte and zero the entry, preventing any hardware updates.
_commit() simply writes the new pte value back knowing that the
hardware has not updated the pte in the meantime.

The only current user of this interface is mprotect

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge[at]citrix.com>
---
include/asm-generic/pgtable.h | 53 +++++++++++++++++++++++++++++++++++++++++
mm/mprotect.c | 10 +++----
2 files changed, 57 insertions(+), 6 deletions(-)

diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
--- a/include/asm-generic/pgtable.h
+++ b/include/asm-generic/pgtable.h
@@ -197,6 +197,59 @@
}
#endif /* CONFIG_MMU */

+static inline pte_t __ptep_modify_prot_start(struct mm_struct *mm,
+ unsigned long addr,
+ pte_t *ptep)
+{
+ /* Get the current pte state, but zero it out to make it
+ non-present, preventing the hardware from asynchronously
+ updating it. */
+ return ptep_get_and_clear(mm, addr, ptep);
+}
+
+static inline void __ptep_modify_prot_commit(struct mm_struct *mm,
+ unsigned long addr,
+ pte_t *ptep, pte_t pte)
+{
+ /* The pte is non-present, so there's no hardware state to
+ preserve. */
+ set_pte_at(mm, addr, ptep, pte);
+}
+
+#ifndef __HAVE_ARCH_PTEP_MODIFY_PROT_TRANSACTION
+/*
+ * Start a pte protection read-modify-write transaction, which
+ * protects against asynchronous hardware modifications to the pte.
+ * The intention is not to prevent the hardware from making pte
+ * updates, but to prevent any updates it may make from being lost.
+ *
+ * This does not protect against other software modifications of the
+ * pte; the appropriate pte lock must be held over the transation.
+ *
+ * Note that this interface is intended to be batchable, meaning that
+ * ptep_modify_prot_commit may not actually update the pte, but merely
+ * queue the update to be done at some later time. The update must be
+ * actually committed before the pte lock is released, however.
+ */
+static inline pte_t ptep_modify_prot_start(struct mm_struct *mm,
+ unsigned long addr,
+ pte_t *ptep)
+{
+ return __ptep_modify_prot_start(mm, addr, ptep);
+}
+
+/*
+ * Commit an update to a pte, leaving any hardware-controlled bits in
+ * the PTE unmodified.
+ */
+static inline void ptep_modify_prot_commit(struct mm_struct *mm,
+ unsigned long addr,
+ pte_t *ptep, pte_t pte)
+{
+ __ptep_modify_prot_commit(mm, addr, ptep, pte);
+}
+#endif /* __HAVE_ARCH_PTEP_MODIFY_PROT_TRANSACTION */
+
/*
* A facility to provide lazy MMU batching. This allows PTE updates and
* page invalidations to be delayed until a call to leave lazy MMU mode
diff --git a/mm/mprotect.c b/mm/mprotect.c
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -47,19 +47,17 @@
if (pte_present(oldpte)) {
pte_t ptent;

- /* Avoid an SMP race with hardware updated dirty/clean
- * bits by wiping the pte and then setting the new pte
- * into place.
- */
- ptent = ptep_get_and_clear(mm, addr, pte);
+ ptent = ptep_modify_prot_start(mm, addr, pte);
ptent = pte_modify(ptent, newprot);
+
/*
* Avoid taking write faults for pages we know to be
* dirty.
*/
if (dirty_accountable && pte_dirty(ptent))
ptent = pte_mkwrite(ptent);
- set_pte_at(mm, addr, pte, ptent);
+
+ ptep_modify_prot_commit(mm, addr, pte, ptent);
#ifdef CONFIG_MIGRATION
} else if (!pte_file(oldpte)) {
swp_entry_t entry = pte_to_swp_entry(oldpte);


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


mingo at elte

Jun 2, 2008, 4:13 AM

Post #2 of 26 (554 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

* Jeremy Fitzhardinge <jeremy[at]goop.org> wrote:

> + /* Get the current pte state, but zero it out to make it
> + non-present, preventing the hardware from asynchronously
> + updating it. */

please use standard and consistent comment style, similar to:

> +/*
> + * Start a pte protection read-modify-write transaction, which
> + * protects against asynchronous hardware modifications to the pte.
> + * The intention is not to prevent the hardware from making pte
> + * updates, but to prevent any updates it may make from being lost.

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


jeremy at goop

Jun 2, 2008, 4:57 AM

Post #3 of 26 (552 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

Ingo Molnar wrote:
> * Jeremy Fitzhardinge <jeremy[at]goop.org> wrote:
>
>
>> + /* Get the current pte state, but zero it out to make it
>> + non-present, preventing the hardware from asynchronously
>> + updating it. */
>>
>
> please use standard and consistent comment style, similar to:
>

OK.

J
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


hugh at veritas

Jun 16, 2008, 11:13 AM

Post #4 of 26 (534 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

On Mon, 16 Jun 2008, Linus Torvalds wrote:
> On Mon, 16 Jun 2008, Jeremy Fitzhardinge wrote:
> >
> > ptep_modify_prot_start() returns the current pte value, and puts the
> > pte entry into a state where either the hardware will not update the
> > pte, or if it does, the updates will be preserved on commit.
> >
> > ptep_modify_prot_commit() writes back the updated pte, makes sure that
> > any hardware updates made since ptep_modify_prot_start() are
> > preserved.
>
> Ok, I'm fine with this now that it's renamed to be clearly about just
> protection bits.
>
> So
>
> Acked-by: Linus Torvalds <torvalds[at]linux-foundation.org>

And seems very reasonable (and exceptionally well described) to me too.

Acked-by: Hugh Dickins <hugh[at]veritas.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


mingo at elte

Jun 16, 2008, 11:49 AM

Post #5 of 26 (534 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

* Hugh Dickins <hugh[at]veritas.com> wrote:

> On Mon, 16 Jun 2008, Linus Torvalds wrote:
> > On Mon, 16 Jun 2008, Jeremy Fitzhardinge wrote:
> > >
> > > ptep_modify_prot_start() returns the current pte value, and puts the
> > > pte entry into a state where either the hardware will not update the
> > > pte, or if it does, the updates will be preserved on commit.
> > >
> > > ptep_modify_prot_commit() writes back the updated pte, makes sure that
> > > any hardware updates made since ptep_modify_prot_start() are
> > > preserved.
> >
> > Ok, I'm fine with this now that it's renamed to be clearly about just
> > protection bits.
> >
> > So
> >
> > Acked-by: Linus Torvalds <torvalds[at]linux-foundation.org>
>
> And seems very reasonable (and exceptionally well described) to me too.
>
> Acked-by: Hugh Dickins <hugh[at]veritas.com>

thanks guys, i've added the Acked-by's and added a new -tip topic for
this.

The dependencies are a bit tricky and the changes contain mm/ and
include/asm-generic/ bits so lets try a new Git trick here to keep it
all tidy and disciplined for v2.6.27 merging.

So i've created a new tip/mm/xen topic branch (the 85th -tip topic
branch ;-), which is COW-ed off the current tip/x86/xen topic branch [on
which branch these changes have some dependencies], and added these 4
changes to that. The tip/x86/xen (append-only-) topic will continue to
advance as usual, and we likely wont have dependencies on the stuff in
tip/mm/xen. (if there will be such dependencies we can handle that too)

In v2.6.27 we can then offer up the two branches separately for upstream
merge, and tip/x86/xen will still only have x86 and xen changes, not any
mm changes. (Obviously tip/mm/xen will be offered after tip/x86/xen has
gone upstream - so that it will only contain these 4 patches ontop of
already-upstream changes)

it will all be auto-merged into linux-next so there this internal
structure is not visible, all the changes are available for wide testing
of course.

i've added these mm/ changes to auto-core-next (not auto-x86-next), if
that is fine by Andrew.

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


jeremy at goop

Jun 18, 2008, 5:37 PM

Post #6 of 26 (529 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

Linus Torvalds wrote:
> On Wed, 18 Jun 2008, Jeremy Fitzhardinge wrote:
>
>> Along the lines of:
>>
>
> Hell no. There's a reason we have a special set_wrprotect() thing. We can
> do it more efficiently on native hardware by just clearing the bit
> atomically. No need to do the cmpxchg games.
>

It's not cmpxchg, just xchg.

In other words, is:

lock btr $_PAGE_BIT_RW, (%rbx)

much cheaper than

mov $0, %rax
xchg %rax, (%rbx)
and $~_PAGE_RW, %rax
mov %rax, (%rbx)

?

It's the same number of locked RMW operations, so aside from being a few
instructions longer, I think it would be much the same.

I guess the correct answer is "lmbench".

J
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


benh at kernel

Jun 18, 2008, 5:39 PM

Post #7 of 26 (531 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

On Wed, 2008-06-18 at 17:24 -0700, Linus Torvalds wrote:
>
> On Wed, 18 Jun 2008, Jeremy Fitzhardinge wrote:
> >
> > Along the lines of:
>
> Hell no. There's a reason we have a special set_wrprotect() thing. We can
> do it more efficiently on native hardware by just clearing the bit
> atomically. No need to do the cmpxchg games.

But we can't batch ...

Ben.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


torvalds at linux-foundation

Jun 18, 2008, 5:49 PM

Post #8 of 26 (529 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

On Wed, 18 Jun 2008, Jeremy Fitzhardinge wrote:
>
> It's not cmpxchg, just xchg.
> In other words, is:
>
> lock btr $_PAGE_BIT_RW, (%rbx)

Well, we can actually do it as

lock andl $~_PAGE_RW,(%rbx)

although we haven't bothered (I've wanted several times to make
clear_bit() do that, but have never gotten around to it - mainly because
old gcc versions didn't work with __builtin_constant_p() in inline
functions - so you have to do the macro from hell)

And yes, the "lock andl" should be noticeably faster than the xchgl.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


torvalds at linux-foundation

Jun 18, 2008, 9:03 PM

Post #9 of 26 (515 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

On Wed, 18 Jun 2008, Linus Torvalds wrote:
>
> And yes, the "lock andl" should be noticeably faster than the xchgl.

I dunno. Here's a untested (!!) patch that turns constant-bit
set/clear_bit ops into byte mask ops (lock orb/andb).

It's not exactly pretty. The reason for using the byte versions is that a
locked op is serialized in the memory pipeline anyway, so there are no
forwarding issues (that could slow down things when we access things with
different sizes), and the byte ops are a lot smaller than 32-bit and
particularly 64-bit ops (big constants, and the 64-bit ops need the REX
prefix byte too).

[. Side note: I wonder if we should turn the "test_bit()" C version into a
"char *" version too.. It could actually help with alias analysis, since
char pointers can alias anything. So it might be the RightThing(tm) to
do for multiple reasons. I dunno. It's a separate issue. ]

It does actually shrink the kernel image a bit (a couple of hundred bytes
on the text segment for my everything-compiled-in image), and while it's
totally untested the (admittedly few) code generation points I looked at
seemed sane. And "lock orb" should be noticeably faster than "lock bts".

If somebody wants to play with it, go wild. I didn't do "change_bit()",
because nobody sane uses that thing anyway. I guarantee nothing. And if it
breaks, nobody saw me do anything. You can't prove this email wasn't sent
by somebody who is good at forging smtp.

This does require a gcc that is recent enough for "__builtin_constant_p()"
to work in an inline function, but I suspect our kernel requirements are
already higher than that. And if you do have an old gcc that is supported,
the worst that would happen is that the optimization doesn't trigger.

Linus

---
include/asm-x86/bitops.h | 27 ++++++++++++++++++++++-----
1 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/include/asm-x86/bitops.h b/include/asm-x86/bitops.h
index ee4b3ea..c1b7f91 100644
--- a/include/asm-x86/bitops.h
+++ b/include/asm-x86/bitops.h
@@ -23,11 +23,22 @@
#if __GNUC__ < 4 || (__GNUC__ == 4 && __GNUC_MINOR__ < 1)
/* Technically wrong, but this avoids compilation errors on some gcc
versions. */
-#define ADDR "=m" (*(volatile long *) addr)
+#define BITOP_ADDR(x) "=m" (*(volatile long *) (x))
#else
-#define ADDR "+m" (*(volatile long *) addr)
+#define BITOP_ADDR(x) "+m" (*(volatile long *) (x))
#endif

+#define ADDR BITOP_ADDR(addr)
+
+/*
+ * We do the locked ops that don't return the old value as
+ * a mask operation on a byte.
+ */
+#define IS_IMMEDIATE(nr) \
+ (__builtin_constant_p(nr))
+#define CONST_MASK_ADDR BITOP_ADDR(addr + (nr>>3))
+#define CONST_MASK (1 << (nr & 7))
+
/**
* set_bit - Atomically set a bit in memory
* @nr: the bit to set
@@ -43,9 +54,12 @@
* Note that @nr may be almost arbitrarily large; this function is not
* restricted to acting on a single-word quantity.
*/
-static inline void set_bit(int nr, volatile void *addr)
+static inline void set_bit(unsigned int nr, volatile void *addr)
{
- asm volatile(LOCK_PREFIX "bts %1,%0" : ADDR : "Ir" (nr) : "memory");
+ if (IS_IMMEDIATE(nr))
+ asm volatile(LOCK_PREFIX "orb %1,%0" : CONST_MASK_ADDR : "i" (CONST_MASK) : "memory");
+ else
+ asm volatile(LOCK_PREFIX "bts %1,%0" : ADDR : "Ir" (nr) : "memory");
}

/**
@@ -74,7 +88,10 @@ static inline void __set_bit(int nr, volatile void *addr)
*/
static inline void clear_bit(int nr, volatile void *addr)
{
- asm volatile(LOCK_PREFIX "btr %1,%0" : ADDR : "Ir" (nr));
+ if (IS_IMMEDIATE(nr))
+ asm volatile(LOCK_PREFIX "andb %1,%0" : CONST_MASK_ADDR : "i" (~CONST_MASK));
+ else
+ asm volatile(LOCK_PREFIX "btr %1,%0" : ADDR : "Ir" (nr));
}

/*
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


jeremy at goop

Jun 18, 2008, 10:03 PM

Post #10 of 26 (515 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

Benjamin Herrenschmidt wrote:
> On Wed, 2008-06-18 at 17:24 -0700, Linus Torvalds wrote:
>
>> On Wed, 18 Jun 2008, Jeremy Fitzhardinge wrote:
>>
>>> Along the lines of:
>>>
>> Hell no. There's a reason we have a special set_wrprotect() thing. We can
>> do it more efficiently on native hardware by just clearing the bit
>> atomically. No need to do the cmpxchg games.
>>
>
> But we can't batch ...
>

Which architecture are you interested in? If it isn't x86, you can
probably get anything past Linus ;)

I'll do some measurements to see what effect the batchable
ptep_set_wrprotect() has on native. If it's significant, I'll propose
making it conditional on CONFIG_PARAVIRT.

J
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


benh at kernel

Jun 19, 2008, 12:20 AM

Post #11 of 26 (511 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

> Which architecture are you interested in? If it isn't x86, you can
> probably get anything past Linus ;)
>
> I'll do some measurements to see what effect the batchable
> ptep_set_wrprotect() has on native. If it's significant, I'll propose
> making it conditional on CONFIG_PARAVIRT.

Oh, I mostly think about powerpc, I just wondered if I could re-use
your new stuff in that context. Mostly idle thoughts at this stage, I
haven't looked seriously.

I have an old patch set to batch forks (and mprotect etc...) TLB
invalidations (which is what I really want to batch on powerpc, more
than the actual PTE changes) that involves subtle changes to the
batching mechanisms etc... but it has issues and would need to be
reworked before merging.

It's something that's been in the back of my mind or the bottom of my
TODO list for some time, but I never quite found the bandwidth to go
back to it.

Cheers,
Ben.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


mingo at elte

Jun 19, 2008, 4:58 AM

Post #12 of 26 (508 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

* Linus Torvalds <torvalds[at]linux-foundation.org> wrote:

> On Wed, 18 Jun 2008, Linus Torvalds wrote:
> >
> > And yes, the "lock andl" should be noticeably faster than the xchgl.
>
> I dunno. Here's a untested (!!) patch that turns constant-bit
> set/clear_bit ops into byte mask ops (lock orb/andb).
>
> It's not exactly pretty. The reason for using the byte versions is
> that a locked op is serialized in the memory pipeline anyway, so there
> are no forwarding issues (that could slow down things when we access
> things with different sizes), and the byte ops are a lot smaller than
> 32-bit and particularly 64-bit ops (big constants, and the 64-bit ops
> need the REX prefix byte too).
>
> [. Side note: I wonder if we should turn the "test_bit()" C version into a
> "char *" version too.. It could actually help with alias analysis, since
> char pointers can alias anything. So it might be the RightThing(tm) to
> do for multiple reasons. I dunno. It's a separate issue. ]
>
> It does actually shrink the kernel image a bit (a couple of hundred bytes
> on the text segment for my everything-compiled-in image), and while it's
> totally untested the (admittedly few) code generation points I looked at
> seemed sane. And "lock orb" should be noticeably faster than "lock bts".
>
> If somebody wants to play with it, go wild. I didn't do
> "change_bit()", because nobody sane uses that thing anyway. I
> guarantee nothing. And if it breaks, nobody saw me do anything. You
> can't prove this email wasn't sent by somebody who is good at forging
> smtp.

i stuck this into tip/x86/bitops branch for kicks - and it blew up very
quickly on 32-bit ;-)

The crash manifests itself as a flood of spurious irqs:

[ 0.997179] irq 96, desc: 788ddd80, depth: 1, count: 0, unhandled: 0
[ 1.003414] ->handle_irq(): 7814c888, handle_bad_irq+0x0/0x1b0
[ 1.003414] ->chip(): 78867174, no_irq_chip+0x0/0x40
[ 1.008350] ->action(): 00000000
[ 1.008350] IRQ_DISABLED set
[ 1.010339] unexpected IRQ trap at vector 60

unappying that change makes the system boot up fine.

a quick guess would be:

io_apic_32.c: if (test_and_set_bit(vector, used_vectors))

io_apic_32.c: set_bit(i, used_vectors);

config and crashlog can be found at:

http://redhat.com/~mingo/misc/config-Thu_Jun_19_13_45_21_CEST_2008.bad
http://redhat.com/~mingo/misc/crash-Thu_Jun_19_13_45_21_CEST_2008.log

Below is the commit, it needed a small amount of massaging to apply the
void * -> unsigned long * change in the x86/bitops topic.

Ingo

-------------->
commit 1a750e0cd7a30c478723ecfa1df685efcdd38a90
Author: Linus Torvalds <torvalds[at]linux-foundation.org>
Date: Wed Jun 18 21:03:26 2008 -0700

x86, bitops: make constant-bit set/clear_bit ops faster

On Wed, 18 Jun 2008, Linus Torvalds wrote:
>
> And yes, the "lock andl" should be noticeably faster than the xchgl.

I dunno. Here's a untested (!!) patch that turns constant-bit
set/clear_bit ops into byte mask ops (lock orb/andb).

It's not exactly pretty. The reason for using the byte versions is that a
locked op is serialized in the memory pipeline anyway, so there are no
forwarding issues (that could slow down things when we access things with
different sizes), and the byte ops are a lot smaller than 32-bit and
particularly 64-bit ops (big constants, and the 64-bit ops need the REX
prefix byte too).

[. Side note: I wonder if we should turn the "test_bit()" C version into a
"char *" version too.. It could actually help with alias analysis, since
char pointers can alias anything. So it might be the RightThing(tm) to
do for multiple reasons. I dunno. It's a separate issue. ]

It does actually shrink the kernel image a bit (a couple of hundred bytes
on the text segment for my everything-compiled-in image), and while it's
totally untested the (admittedly few) code generation points I looked at
seemed sane. And "lock orb" should be noticeably faster than "lock bts".

If somebody wants to play with it, go wild. I didn't do "change_bit()",
because nobody sane uses that thing anyway. I guarantee nothing. And if it
breaks, nobody saw me do anything. You can't prove this email wasn't sent
by somebody who is good at forging smtp.

This does require a gcc that is recent enough for "__builtin_constant_p()"
to work in an inline function, but I suspect our kernel requirements are
already higher than that. And if you do have an old gcc that is supported,
the worst that would happen is that the optimization doesn't trigger.

Signed-off-by: Ingo Molnar <mingo[at]elte.hu>

diff --git a/include/asm-x86/bitops.h b/include/asm-x86/bitops.h
index 7d2494b..ab7635a 100644
--- a/include/asm-x86/bitops.h
+++ b/include/asm-x86/bitops.h
@@ -23,11 +23,22 @@
#if __GNUC__ < 4 || (__GNUC__ == 4 && __GNUC_MINOR__ < 1)
/* Technically wrong, but this avoids compilation errors on some gcc
versions. */
-#define ADDR "=m" (*(volatile long *) addr)
+#define BITOP_ADDR(x) "=m" (*(volatile long *) (x))
#else
-#define ADDR "+m" (*(volatile long *) addr)
+#define BITOP_ADDR(x) "+m" (*(volatile long *) (x))
#endif

+#define ADDR BITOP_ADDR(addr)
+
+/*
+ * We do the locked ops that don't return the old value as
+ * a mask operation on a byte.
+ */
+#define IS_IMMEDIATE(nr) \
+ (__builtin_constant_p(nr))
+#define CONST_MASK_ADDR BITOP_ADDR(addr + (nr>>3))
+#define CONST_MASK (1 << (nr & 7))
+
/**
* set_bit - Atomically set a bit in memory
* @nr: the bit to set
@@ -43,11 +54,15 @@
* Note that @nr may be almost arbitrarily large; this function is not
* restricted to acting on a single-word quantity.
*/
-static inline void set_bit(int nr, volatile unsigned long *addr)
+static inline void set_bit(unsigned int nr, volatile unsigned long *addr)
{
- asm volatile(LOCK_PREFIX "bts %1,%0" : ADDR : "Ir" (nr) : "memory");
+ if (IS_IMMEDIATE(nr))
+ asm volatile(LOCK_PREFIX "orb %1,%0" : CONST_MASK_ADDR : "i" (CONST_MASK) : "memory");
+ else
+ asm volatile(LOCK_PREFIX "bts %1,%0" : ADDR : "Ir" (nr) : "memory");
}

+
/**
* __set_bit - Set a bit in memory
* @nr: the bit to set
@@ -74,7 +89,10 @@ static inline void __set_bit(int nr, volatile unsigned long *addr)
*/
static inline void clear_bit(int nr, volatile unsigned long *addr)
{
- asm volatile(LOCK_PREFIX "btr %1,%0" : ADDR : "Ir" (nr));
+ if (IS_IMMEDIATE(nr))
+ asm volatile(LOCK_PREFIX "andb %1,%0" : CONST_MASK_ADDR : "i" (~CONST_MASK));
+ else
+ asm volatile(LOCK_PREFIX "btr %1,%0" : ADDR : "Ir" (nr));
}

/*
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


mingo at elte

Jun 19, 2008, 5:03 AM

Post #13 of 26 (507 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

* Ingo Molnar <mingo[at]elte.hu> wrote:

> config and crashlog can be found at:
>
> http://redhat.com/~mingo/misc/config-Thu_Jun_19_13_45_21_CEST_2008.bad
> http://redhat.com/~mingo/misc/crash-Thu_Jun_19_13_45_21_CEST_2008.log

just in case it helps, and for completeness, a 64-bit box blew up too:

http://redhat.com/~mingo/misc/config-Thu_Jun_19_13_48_55_CEST_2008.bad

and another 32-bit box:

http://redhat.com/~mingo/misc/config-Thu_Jun_19_13_48_32_CEST_2008.bad

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


akinobu.mita at gmail

Jun 19, 2008, 5:20 AM

Post #14 of 26 (509 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

> Below is the commit, it needed a small amount of massaging to apply the
> void * -> unsigned long * change in the x86/bitops topic.

So you need to change this line

> +#define CONST_MASK_ADDR BITOP_ADDR(addr + (nr>>3))

to be

#define CONST_MASK_ADDR BITOP_ADDR((void *)addr + (nr>>3))

or something like this. Otherwise it will get wrong address in set_bit

> -static inline void set_bit(int nr, volatile unsigned long *addr)
> +static inline void set_bit(unsigned int nr, volatile unsigned long *addr)
> {
> - asm volatile(LOCK_PREFIX "bts %1,%0" : ADDR : "Ir" (nr) : "memory");
> + if (IS_IMMEDIATE(nr))
> + asm volatile(LOCK_PREFIX "orb %1,%0" : CONST_MASK_ADDR : "i" (CONST_MASK) : "memory");
> + else
> + asm volatile(LOCK_PREFIX "bts %1,%0" : ADDR : "Ir" (nr) : "memory");
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


torvalds at linux-foundation

Jun 19, 2008, 9:30 AM

Post #15 of 26 (509 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

On Thu, 19 Jun 2008, Ingo Molnar wrote:
>
> Below is the commit, it needed a small amount of massaging to apply the
> void * -> unsigned long * change in the x86/bitops topic.

Well, that's your bug right there.

The macros very much depended on the pointers being "void *", due to the
pointer arithmetic (which is a gcc extension that we use extensively -
"void *" arithmetic works as if it was a byte pointer).

When you changed "addr" to "volatile unsigned long *", now the arithmetic
ended up multiplying the offset by the size of "long" before adding it.

That said, the _original_ patch wasn't tested either, but quite frankly,
looking over the patch one more time, I can't really see how it could
break.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


mingo at elte

Jun 19, 2008, 9:47 AM

Post #16 of 26 (510 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

* Linus Torvalds <torvalds[at]linux-foundation.org> wrote:

> On Thu, 19 Jun 2008, Ingo Molnar wrote:
> >
> > Below is the commit, it needed a small amount of massaging to apply the
> > void * -> unsigned long * change in the x86/bitops topic.
>
> Well, that's your bug right there.
>
> The macros very much depended on the pointers being "void *", due to
> the pointer arithmetic (which is a gcc extension that we use
> extensively - "void *" arithmetic works as if it was a byte pointer).

duh, yeah - of course. Will retry with that fixed :)

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


jeremy at goop

Jun 19, 2008, 10:57 AM

Post #17 of 26 (502 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

Benjamin Herrenschmidt wrote:
>> Which architecture are you interested in? If it isn't x86, you can
>> probably get anything past Linus ;)
>>
>> I'll do some measurements to see what effect the batchable
>> ptep_set_wrprotect() has on native. If it's significant, I'll propose
>> making it conditional on CONFIG_PARAVIRT.
>>
>
> Oh, I mostly think about powerpc, I just wondered if I could re-use
> your new stuff in that context. Mostly idle thoughts at this stage, I
> haven't looked seriously.
>

There are general-purpose hooks in the common code which architectures
can implement however they like. In x86-land we hook them up to pvops,
but you can handle them however you want.

> I have an old patch set to batch forks (and mprotect etc...) TLB
> invalidations (which is what I really want to batch on powerpc, more
> than the actual PTE changes) that involves subtle changes to the
> batching mechanisms etc...
>

Do you mean setting up batches of per-page tlb shootdowns rather than
going a global tlb flush at the end?

J
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


mingo at elte

Jun 20, 2008, 3:10 AM

Post #18 of 26 (488 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

* Ingo Molnar <mingo[at]elte.hu> wrote:

>
> * Linus Torvalds <torvalds[at]linux-foundation.org> wrote:
>
> > On Thu, 19 Jun 2008, Ingo Molnar wrote:
> > >
> > > Below is the commit, it needed a small amount of massaging to apply the
> > > void * -> unsigned long * change in the x86/bitops topic.
> >
> > Well, that's your bug right there.
> >
> > The macros very much depended on the pointers being "void *", due to
> > the pointer arithmetic (which is a gcc extension that we use
> > extensively - "void *" arithmetic works as if it was a byte
> > pointer).
>
> duh, yeah - of course. Will retry with that fixed :)

yep, the patch below got it all going and it passed 5 hours of testing
already. Thanks,

Ingo

------------------->
commit 7dbceaf9bb68919651901b101f44edd5391ee489
Author: Ingo Molnar <mingo[at]elte.hu>
Date: Fri Jun 20 07:28:24 2008 +0200

x86, bitops: make constant-bit set/clear_bit ops faster, adapt, clean up

fix integration bug introduced by "x86: bitops take an unsigned long *"
which turned "(void *) + x" into "(long *) + x".

small cleanups to make it more apparent which value get propagated where.

Signed-off-by: Ingo Molnar <mingo[at]elte.hu>

diff --git a/include/asm-x86/bitops.h b/include/asm-x86/bitops.h
index ab7635a..6c50548 100644
--- a/include/asm-x86/bitops.h
+++ b/include/asm-x86/bitops.h
@@ -28,16 +28,15 @@
#define BITOP_ADDR(x) "+m" (*(volatile long *) (x))
#endif

-#define ADDR BITOP_ADDR(addr)
+#define ADDR BITOP_ADDR(addr)

/*
* We do the locked ops that don't return the old value as
* a mask operation on a byte.
*/
-#define IS_IMMEDIATE(nr) \
- (__builtin_constant_p(nr))
-#define CONST_MASK_ADDR BITOP_ADDR(addr + (nr>>3))
-#define CONST_MASK (1 << (nr & 7))
+#define IS_IMMEDIATE(nr) (__builtin_constant_p(nr))
+#define CONST_MASK_ADDR(nr, addr) BITOP_ADDR((void *)(addr) + ((nr)>>3))
+#define CONST_MASK(nr) (1 << ((nr) & 7))

/**
* set_bit - Atomically set a bit in memory
@@ -56,13 +55,17 @@
*/
static inline void set_bit(unsigned int nr, volatile unsigned long *addr)
{
- if (IS_IMMEDIATE(nr))
- asm volatile(LOCK_PREFIX "orb %1,%0" : CONST_MASK_ADDR : "i" (CONST_MASK) : "memory");
- else
- asm volatile(LOCK_PREFIX "bts %1,%0" : ADDR : "Ir" (nr) : "memory");
+ if (IS_IMMEDIATE(nr)) {
+ asm volatile(LOCK_PREFIX "orb %1,%0"
+ : CONST_MASK_ADDR(nr, addr)
+ : "i" (CONST_MASK(nr))
+ : "memory");
+ } else {
+ asm volatile(LOCK_PREFIX "bts %1,%0"
+ : BITOP_ADDR(addr) : "Ir" (nr) : "memory");
+ }
}

-
/**
* __set_bit - Set a bit in memory
* @nr: the bit to set
@@ -89,10 +92,15 @@ static inline void __set_bit(int nr, volatile unsigned long *addr)
*/
static inline void clear_bit(int nr, volatile unsigned long *addr)
{
- if (IS_IMMEDIATE(nr))
- asm volatile(LOCK_PREFIX "andb %1,%0" : CONST_MASK_ADDR : "i" (~CONST_MASK));
- else
- asm volatile(LOCK_PREFIX "btr %1,%0" : ADDR : "Ir" (nr));
+ if (IS_IMMEDIATE(nr)) {
+ asm volatile(LOCK_PREFIX "andb %1,%0"
+ : CONST_MASK_ADDR(nr, addr)
+ : "i" (~CONST_MASK(nr)));
+ } else {
+ asm volatile(LOCK_PREFIX "btr %1,%0"
+ : BITOP_ADDR(addr)
+ : "Ir" (nr));
+ }
}

/*
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


jeremy at goop

Jun 20, 2008, 12:06 PM

Post #19 of 26 (485 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

Ingo Molnar wrote:
> * Ingo Molnar <mingo[at]elte.hu> wrote:
>
>
>> * Linus Torvalds <torvalds[at]linux-foundation.org> wrote:
>>
>>
>>> On Thu, 19 Jun 2008, Ingo Molnar wrote:
>>>
>>>> Below is the commit, it needed a small amount of massaging to apply the
>>>> void * -> unsigned long * change in the x86/bitops topic.
>>>>
>>> Well, that's your bug right there.
>>>
>>> The macros very much depended on the pointers being "void *", due to
>>> the pointer arithmetic (which is a gcc extension that we use
>>> extensively - "void *" arithmetic works as if it was a byte
>>> pointer).
>>>
>> duh, yeah - of course. Will retry with that fixed :)
>>
>
> yep, the patch below got it all going and it passed 5 hours of testing
> already. Thanks,
>

Blows up on "gcc version 3.4.4 20050314 (prerelease) (Debian 3.4.3-13)":

CC init/main.o
include2/asm/bitops.h: In function `start_kernel':
include2/asm/bitops.h:59: warning: asm operand 1 probably doesn't match constraints
include2/asm/bitops.h:59: warning: asm operand 1 probably doesn't match constraints
include2/asm/bitops.h:59: warning: asm operand 1 probably doesn't match constraints
include2/asm/bitops.h:59: error: impossible constraint in `asm'
include2/asm/bitops.h:59: error: impossible constraint in `asm'
include2/asm/bitops.h:59: error: impossible constraint in `asm'

J


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


torvalds at linux-foundation

Jun 20, 2008, 12:15 PM

Post #20 of 26 (485 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

On Fri, 20 Jun 2008, Jeremy Fitzhardinge wrote:
>
> Blows up on "gcc version 3.4.4 20050314 (prerelease) (Debian 3.4.3-13)":

Yeah, I was a bit worried about that. Gcc sometimes does insane things.

We literally just tested that the asm should only _ever_ be generated with
a constant value, but if some gcc dead-code removal thing doesn't work, it
will then screw up and try to generate the asm even for a non-constant
thing.

The fairly trivial fix is probably to just change the "i" to "ir", safe in
the knowledge that any _sane_ case will never use the "r" possibility. I
suspect even your insane case will end up then killing the bad choice
later.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


mingo at elte

Jun 20, 2008, 12:56 PM

Post #21 of 26 (485 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

* Linus Torvalds <torvalds[at]linux-foundation.org> wrote:

> On Fri, 20 Jun 2008, Jeremy Fitzhardinge wrote:
> >
> > Blows up on "gcc version 3.4.4 20050314 (prerelease) (Debian 3.4.3-13)":
>
> Yeah, I was a bit worried about that. Gcc sometimes does insane
> things.
>
> We literally just tested that the asm should only _ever_ be generated
> with a constant value, but if some gcc dead-code removal thing doesn't
> work, it will then screw up and try to generate the asm even for a
> non-constant thing.
>
> The fairly trivial fix is probably to just change the "i" to "ir",
> safe in the knowledge that any _sane_ case will never use the "r"
> possibility. I suspect even your insane case will end up then killing
> the bad choice later.

okay - Jeremy, could you try the fix below? (or tip/master, i just
pushed this out)

(i dont use gcc 3.x myself to build the kernel, had way too many
miscompilations in randconfig tests in the past.)

Ingo

-------------->
commit b68b80b8ab39c42707dc126c41e87d46edc97c27
Author: Ingo Molnar <mingo[at]elte.hu>
Date: Fri Jun 20 21:50:20 2008 +0200

x86, bitops: make constant-bit set/clear_bit ops faster, gcc workaround

Jeremy Fitzhardinge reported this compiler bug:

Suggestion from Linus: add "r" to the input constraint of the
set_bit()/clear_bit()'s constant 'nr' branch:

Blows up on "gcc version 3.4.4 20050314 (prerelease) (Debian 3.4.3-13)":

CC init/main.o
include2/asm/bitops.h: In function `start_kernel':
include2/asm/bitops.h:59: warning: asm operand 1 probably doesn't match constraints
include2/asm/bitops.h:59: warning: asm operand 1 probably doesn't match constraints
include2/asm/bitops.h:59: warning: asm operand 1 probably doesn't match constraints
include2/asm/bitops.h:59: error: impossible constraint in `asm'
include2/asm/bitops.h:59: error: impossible constraint in `asm'
include2/asm/bitops.h:59: error: impossible constraint in `asm'

Reported-by: Jeremy Fitzhardinge <jeremy[at]goop.org>
Signed-off-by: Ingo Molnar <mingo[at]elte.hu>

diff --git a/include/asm-x86/bitops.h b/include/asm-x86/bitops.h
index 6c50548..4575de4 100644
--- a/include/asm-x86/bitops.h
+++ b/include/asm-x86/bitops.h
@@ -58,7 +58,7 @@ static inline void set_bit(unsigned int nr, volatile unsigned long *addr)
if (IS_IMMEDIATE(nr)) {
asm volatile(LOCK_PREFIX "orb %1,%0"
: CONST_MASK_ADDR(nr, addr)
- : "i" (CONST_MASK(nr))
+ : "ir" (CONST_MASK(nr))
: "memory");
} else {
asm volatile(LOCK_PREFIX "bts %1,%0"
@@ -95,7 +95,7 @@ static inline void clear_bit(int nr, volatile unsigned long *addr)
if (IS_IMMEDIATE(nr)) {
asm volatile(LOCK_PREFIX "andb %1,%0"
: CONST_MASK_ADDR(nr, addr)
- : "i" (~CONST_MASK(nr)));
+ : "ir" (~CONST_MASK(nr)));
} else {
asm volatile(LOCK_PREFIX "btr %1,%0"
: BITOP_ADDR(addr)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


torvalds at linux-foundation

Jun 20, 2008, 1:03 PM

Post #22 of 26 (486 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

On Fri, 20 Jun 2008, Ingo Molnar wrote:
>
> okay - Jeremy, could you try the fix below? (or tip/master, i just
> pushed this out)

Actually, don't try that one.

It needs to be a _byte_ registers, so "ir" was wrong. You need "iq".

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


jeremy at goop

Jun 20, 2008, 1:05 PM

Post #23 of 26 (485 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

Ingo Molnar wrote:
> * Linus Torvalds <torvalds[at]linux-foundation.org> wrote:
>
>
>> On Fri, 20 Jun 2008, Jeremy Fitzhardinge wrote:
>>
>>> Blows up on "gcc version 3.4.4 20050314 (prerelease) (Debian 3.4.3-13)":
>>>
>> Yeah, I was a bit worried about that. Gcc sometimes does insane
>> things.
>>
>> We literally just tested that the asm should only _ever_ be generated
>> with a constant value, but if some gcc dead-code removal thing doesn't
>> work, it will then screw up and try to generate the asm even for a
>> non-constant thing.
>>
>> The fairly trivial fix is probably to just change the "i" to "ir",
>> safe in the knowledge that any _sane_ case will never use the "r"
>> possibility. I suspect even your insane case will end up then killing
>> the bad choice later.
>>
>
> okay - Jeremy, could you try the fix below? (or tip/master, i just
> pushed this out)
>
>

Hm. On 32-bit I get these, but they're warnings. On 64-bit they're errors.

{standard input}: Assembler messages:
{standard input}:1609: Warning: using `%dl' instead of `%edx' due to `b'
suffix

vs

{standard input}: Assembler messages:
{standard input}:20511: Error: Incorrect register `%eax' used with `b'
suffix


> (i dont use gcc 3.x myself to build the kernel, had way too many
> miscompilations in randconfig tests in the past.)
>

I do it mainly to pick up these kinds of problems.


> Ingo
>
> -------------->
> commit b68b80b8ab39c42707dc126c41e87d46edc97c27
> Author: Ingo Molnar <mingo[at]elte.hu>
> Date: Fri Jun 20 21:50:20 2008 +0200
>
> x86, bitops: make constant-bit set/clear_bit ops faster, gcc workaround
>
> Jeremy Fitzhardinge reported this compiler bug:
>
> Suggestion from Linus: add "r" to the input constraint of the
> set_bit()/clear_bit()'s constant 'nr' branch:
>
> Blows up on "gcc version 3.4.4 20050314 (prerelease) (Debian 3.4.3-13)":
>
> CC init/main.o
> include2/asm/bitops.h: In function `start_kernel':
> include2/asm/bitops.h:59: warning: asm operand 1 probably doesn't match constraints
> include2/asm/bitops.h:59: warning: asm operand 1 probably doesn't match constraints
> include2/asm/bitops.h:59: warning: asm operand 1 probably doesn't match constraints
> include2/asm/bitops.h:59: error: impossible constraint in `asm'
> include2/asm/bitops.h:59: error: impossible constraint in `asm'
> include2/asm/bitops.h:59: error: impossible constraint in `asm'
>
> Reported-by: Jeremy Fitzhardinge <jeremy[at]goop.org>
> Signed-off-by: Ingo Molnar <mingo[at]elte.hu>
>
> diff --git a/include/asm-x86/bitops.h b/include/asm-x86/bitops.h
> index 6c50548..4575de4 100644
> --- a/include/asm-x86/bitops.h
> +++ b/include/asm-x86/bitops.h
> @@ -58,7 +58,7 @@ static inline void set_bit(unsigned int nr, volatile unsigned long *addr)
> if (IS_IMMEDIATE(nr)) {
> asm volatile(LOCK_PREFIX "orb %1,%0"
> : CONST_MASK_ADDR(nr, addr)
> - : "i" (CONST_MASK(nr))
> + : "ir" (CONST_MASK(nr))
> : "memory");
> } else {
> asm volatile(LOCK_PREFIX "bts %1,%0"
> @@ -95,7 +95,7 @@ static inline void clear_bit(int nr, volatile unsigned long *addr)
> if (IS_IMMEDIATE(nr)) {
> asm volatile(LOCK_PREFIX "andb %1,%0"
> : CONST_MASK_ADDR(nr, addr)
> - : "i" (~CONST_MASK(nr)));
> + : "ir" (~CONST_MASK(nr)));
> } else {
> asm volatile(LOCK_PREFIX "btr %1,%0"
> : BITOP_ADDR(addr)
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


jeremy at goop

Jun 20, 2008, 1:16 PM

Post #24 of 26 (484 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

Linus Torvalds wrote:
> On Fri, 20 Jun 2008, Ingo Molnar wrote:
>
>> okay - Jeremy, could you try the fix below? (or tip/master, i just
>> pushed this out)
>>
>
> Actually, don't try that one.
>
> It needs to be a _byte_ registers, so "ir" was wrong. You need "iq".
>

Doesn't work, unfortunately:
{standard input}:20511: Error: Incorrect register `%eax' used with `b'
suffix

lock; orb %eax,1(%rdi) # tmp64,

J
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


jeremy at goop

Jun 20, 2008, 1:22 PM

Post #25 of 26 (486 views)
Permalink
Re: [PATCH 1 of 4] mm: add a ptep_modify_prot transaction abstraction [In reply to]

Jeremy Fitzhardinge wrote:
> Linus Torvalds wrote:
>
>> On Fri, 20 Jun 2008, Ingo Molnar wrote:
>>
>>
>>> okay - Jeremy, could you try the fix below? (or tip/master, i just
>>> pushed this out)
>>>
>>>
>> Actually, don't try that one.
>>
>> It needs to be a _byte_ registers, so "ir" was wrong. You need "iq".
>>
>>
>
> Doesn't work, unfortunately:
> {standard input}:20511: Error: Incorrect register `%eax' used with `b'
> suffix
>
> lock; orb %eax,1(%rdi) # tmp64,
>

This does work:

asm volatile(LOCK_PREFIX "orb %1,%0"
: CONST_MASK_ADDR(nr, addr)
: "iq" ((u8)CONST_MASK(nr))
: "memory");

(ie, explicitly casting the mask to u8)

J
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

First page Previous page 1 2 Next page Last page  View All Linux kernel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact lists@gossamer-threads.com
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.