[RFC] should VM_BUG_ON(cond) really evaluate cond

[RFC] should VM_BUG_ON(cond) really evaluate cond

[Eric Dumazet]

In commit 4e60c86bd9e (gcc-4.6: mm: fix unused but set warnings)
Andi forced VM_BUG_ON(cond) to evaluate cond, even if CONFIG_DEBUG_VM is
not set :

#ifdef CONFIG_DEBUG_VM
#define VM_BUG_ON(cond) BUG_ON(cond)
#else
#define VM_BUG_ON(cond) do { (void)(cond); } while (0)
#endif

As a side effect, get_page()/put_page_testzero() are performing more bus
transactions on contended cache line on some workloads (tcp on loopback
for example, where a page is acting as a shared buffer)

0,05 :  ffffffff815e4775:       je     ffffffff815e4970 <tcp_sendmsg+0xc80>
0,05 :  ffffffff815e477b:       mov    0x1c(%r9),%eax    // useless                  
3,32 :  ffffffff815e477f:       mov    (%r9),%rax        // useless                  
0,51 :  ffffffff815e4782:       lock incl 0x1c(%r9)                        
3,87 :  ffffffff815e4787:       mov    (%r9),%rax                          
0,00 :  ffffffff815e478a:       test   $0x80,%ah                           
0,00 :  ffffffff815e478d:       jne    ffffffff815e49f2 <tcp_sendmsg+0xd02>      

Of course, we have to understand why

(void) (atomic_read(&some_atomic) == 1);

generates asm code...

	mov  some_atomic,%eax

Ah yes, this is because of the volatile...

static inline int atomic_read(const atomic_t *v)
{
	return (*(volatile int *)&(v)->counter);
}

So maybe a fix would be to introduce an atomic_read_stable() variant ?

static inline int atomic_read_stable(const atomic_t *v)
{
	return v->counter;
}

Thanks !

Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Andi Kleen]

On Fri, Oct 28, 2011 at 03:19:21AM +0200, Eric Dumazet wrote:
> In commit 4e60c86bd9e (gcc-4.6: mm: fix unused but set warnings)
> Andi forced VM_BUG_ON(cond) to evaluate cond, even if CONFIG_DEBUG_VM is
> not set :
> 
> #ifdef CONFIG_DEBUG_VM
> #define VM_BUG_ON(cond) BUG_ON(cond)
> #else
> #define VM_BUG_ON(cond) do { (void)(cond); } while (0)
> #endif

Eventually the warnings were disabled in the Makefile.
So it would be reasonable to just revert that patch now, at least
for VM_BUG_ON, if it costs performance.

> 
> So maybe a fix would be to introduce an atomic_read_stable() variant ?
> 
> static inline int atomic_read_stable(const atomic_t *v)
> {
> 	return v->counter;
> }

Seems reasonable too. In fact we usually should have memory barriers
for this anyways which obsolete the volatile.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Linus Torvalds]

On Thu, Oct 27, 2011 at 6:25 PM, Andi Kleen <andi@firstfloor.org> wrote:
>
> Seems reasonable too. In fact we usually should have memory barriers
> for this anyways which obsolete the volatile.

No we shouldn't. Memory barriers are insanely expensive, and pointless
for atomics - that aren't ordered anyway.

You may mean compiler barriers.

That said, removing the volatile entirely might be a good idea, and
never mind any barriers at all. The ordering for atomics really isn't
well enough specified that we should care. So I wouldn't object to a
patch that just removes the volatile entirely, but it would have to be
accompanied with quite a bit of testing, in case some odd case ends up
depending on it. But nothing *should* be looping on those things
anyway.

                       Linus

Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Ben Hutchings]

On Thu, 2011-10-27 at 18:34 -0700, Linus Torvalds wrote:
> On Thu, Oct 27, 2011 at 6:25 PM, Andi Kleen <andi@firstfloor.org> wrote:
> >
> > Seems reasonable too. In fact we usually should have memory barriers
> > for this anyways which obsolete the volatile.
> 
> No we shouldn't. Memory barriers are insanely expensive, and pointless
> for atomics - that aren't ordered anyway.
> 
> You may mean compiler barriers.
> 
> That said, removing the volatile entirely might be a good idea, and
> never mind any barriers at all. The ordering for atomics really isn't
> well enough specified that we should care. So I wouldn't object to a
> patch that just removes the volatile entirely, but it would have to be
> accompanied with quite a bit of testing, in case some odd case ends up
> depending on it. But nothing *should* be looping on those things
> anyway.

Whether or not it needs to provide any ordering guarantee, atomic_read()
must never read more than once, and I think that requires the volatile
qualification.  It might be clearer to use the ACCESS_ONCE macro,
however.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Eric Dumazet]

Le vendredi 28 octobre 2011 à 02:44 +0100, Ben Hutchings a écrit :

> Whether or not it needs to provide any ordering guarantee, atomic_read()
> must never read more than once, and I think that requires the volatile
> qualification.  It might be clearer to use the ACCESS_ONCE macro,
> however.
> 

Where this requirement comes from ?

Maybe then introduce atomic_read_once() for users really needing it :)

ACCESS_ONCE will force the read/move instruction I try to avoid :(

By the way, removing volatile on my x86_64 defconfig saves a bit of
space :

# size vmlinux vmlinux.old
   text	   data	    bss	    dec	    hex	filename
10907585	2890992	1540096	15338673	 ea0cb1	vmlinux
10912342	2891056	1540096	15343494	 ea1f86	vmlinux.old

booted and everything seems fine, but obviously lightly tested...

Note : There is still one useless access to page-flags,
'fixing' atomic_read() is not enough.


    0,04 :        ffffffff815e4675:       je     ffffffff815e4870 <tcp_sendmsg+0xc80>
    0,08 :        ffffffff815e467b:       mov    (%r9),%rax   // useless
    2,08 :        ffffffff815e467e:       lock incl 0x1c(%r9)
    3,58 :        ffffffff815e4683:       mov    (%r9),%rax
    0,04 :        ffffffff815e4686:       test   $0x80,%ah
    0,00 :        ffffffff815e4689:       jne    ffffffff815e48ee <tcp_sendmsg+0xcfe>

Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Ben Hutchings]

On Fri, 2011-10-28 at 04:52 +0200, Eric Dumazet wrote:
> Le vendredi 28 octobre 2011 à 02:44 +0100, Ben Hutchings a écrit :
> 
> > Whether or not it needs to provide any ordering guarantee, atomic_read()
> > must never read more than once, and I think that requires the volatile
> > qualification.  It might be clearer to use the ACCESS_ONCE macro,
> > however.
> > 
> 
> Where this requirement comes from ?

That is the conventional behaviour of 'atomic' operations, and callers
may depend on it.

> Maybe then introduce atomic_read_once() for users really needing it :)
>
> ACCESS_ONCE will force the read/move instruction I try to avoid :(
[...]

I'm sure you can find some other way to avoid it.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

>Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Eric Dumazet]

Le vendredi 28 octobre 2011 à 04:29 +0100, Ben Hutchings a écrit :
> On Fri, 2011-10-28 at 04:52 +0200, Eric Dumazet wrote:
> > Le vendredi 28 octobre 2011 à 02:44 +0100, Ben Hutchings a écrit :
> > 
> > > Whether or not it needs to provide any ordering guarantee, atomic_read()
> > > must never read more than once, and I think that requires the volatile
> > > qualification.  It might be clearer to use the ACCESS_ONCE macro,
> > > however.
> > > 
> > 
> > Where this requirement comes from ?
> 
> That is the conventional behaviour of 'atomic' operations, and callers
> may depend on it.
> 

Thats your opinion, not a fact documented in
Documentation/atomic_ops.txt

<QUOTE>
---------------------------------------------------------
Next, we have:

        #define atomic_read(v)  ((v)->counter)

which simply reads the counter value currently visible to the calling thread.
The read is atomic in that the return value is guaranteed to be one of the
values initialized or modified with the interface operations if a proper
implicit or explicit memory barrier is used after possible runtime
initialization by any other thread and the value is modified only with the
interface operations.  atomic_read does not guarantee that the runtime
initialization by any other thread is visible yet, so the user of the
interface must take care of that with a proper implicit or explicit memory
barrier.

*** WARNING: atomic_read() and atomic_set() DO NOT IMPLY BARRIERS! ***

Some architectures may choose to use the volatile keyword, barriers, or inline
assembly to guarantee some degree of immediacy for atomic_read() and
atomic_set().  This is not uniformly guaranteed, and may change in the future,
so all users of atomic_t should treat atomic_read() and atomic_set() as simple
C statements that may be reordered or optimized away entirely by the compiler
or processor, and explicitly invoke the appropriate compiler and/or memory
barrier for each use case.  Failure to do so will result in code that may
suddenly break when used with different architectures or compiler
optimizations, or even changes in unrelated code which changes how the
compiler optimizes the section accessing atomic_t variables.

*** YOU HAVE BEEN WARNED! ***
---------------------------------------------------------------
</QUOTE>

The only requirement of atomic_read() is that it must return value
before or after an atomic_write(), not a garbled value.

So the ACCESS_ONCE semantic is on the write side (the atomic itself
cannot be used as a temporary variable. It must contain only two value
of atomic_{write|add|sub}() [ prior to the operation, after the
operation ]

In fact, if a compiler is stupid enough to issue two reads on following
code :

static inline atomic_read(const atomic_t *p)
{
	return p->value;
}

Then its fine, because in the end the returned value will be the old or
new one.

Re: >Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Linus Torvalds]

On Thu, Oct 27, 2011 at 9:43 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> The only requirement of atomic_read() is that it must return value
> before or after an atomic_write(), not a garbled value.

The problem is that gcc *can* return a garbled value.

> In fact, if a compiler is stupid enough to issue two reads on following
> code :

The compiler really *can* be that "stupid". Except the code tends to
look like this:

   int value = atomic_read(&atomic_var);
   if (value > 10)
     return;
   .. do something with value ..

and gcc may decide - under register pressure, and in the absense of a
'volatile' - to read 'value' first once for that "> 10" check, and
then it drops the registers and instead of saving it on the stack
frame, it can decide to re-load it from atomic_var.

IOW, "value" could be two or more different values: one value when
testing, and *another* value in "do something with value".

This is why we have "ACCESS_ONCE()".

Whether atomics guarantee ACCESS_ONCE() semantics or not is not
entirely clear. But afaik, there is no way to tell gcc "access at
*most* once, and never ever reload".

                             Linus

Re: >Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Eric Dumazet]

Le vendredi 28 octobre 2011 à 04:37 -0700, Linus Torvalds a écrit :
> On Thu, Oct 27, 2011 at 9:43 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> > The only requirement of atomic_read() is that it must return value
> > before or after an atomic_write(), not a garbled value.
> 
> The problem is that gcc *can* return a garbled value.
> 
> > In fact, if a compiler is stupid enough to issue two reads on following
> > code :
> 
> The compiler really *can* be that "stupid". Except the code tends to
> look like this:
> 
>    int value = atomic_read(&atomic_var);
>    if (value > 10)
>      return;
>    .. do something with value ..
> 
> and gcc may decide - under register pressure, and in the absense of a
> 'volatile' - to read 'value' first once for that "> 10" check, and
> then it drops the registers and instead of saving it on the stack
> frame, it can decide to re-load it from atomic_var.
> 
> IOW, "value" could be two or more different values: one value when
> testing, and *another* value in "do something with value".
> 
> This is why we have "ACCESS_ONCE()".
> 
> Whether atomics guarantee ACCESS_ONCE() semantics or not is not
> entirely clear. But afaik, there is no way to tell gcc "access at
> *most* once, and never ever reload".
> 

What you describe is true for non atomic variables as well, its not part
of the atomic_ops documented semantic.

And we do use ACCESS_ONCE() on the rare cases we need to make sure no
reload is done.

RCU use makes this implied (ACCESS_ONCE() being done in
rcu_dereference()), so we dont have many raw ACCESS_ONCE() in our code.

int value = ACCESS_ONCE(atomic_read(&atomic_var));
if (value > 10)
     return;
.. do something with value ..

One of such rare use is explained in commit f1987257
(tcp: protect sysctl_tcp_cookie_size reads)

Since its a bit ugly, I suggested :

int value = atomic_read_once(&atomic_var);
if (value > 10)
     return;
    .. do something with value ..

I dont know, it seems the right way, but yes it might break things.

We can take the otherway and patch thousand atomic_read() to
atomic_read_stable(), its safer but very boring :)

Re: >Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Linus Torvalds]

On Fri, Oct 28, 2011 at 5:09 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> What you describe is true for non atomic variables as well, its not part
> of the atomic_ops documented semantic.

Eric - your "documentation" is so much toilet paper. So don't bother
talkign about "documented semantics". Start talking about SANE
INTERFACES.

What I described is very much true of non-atomics. Nobody doubts that.

THE QUESTION IS WHETHER IT MAKES SENSE FOR ATOMICS, DAMMIT!

And quite frankly, the behavior I described does not seem to make any
sense for atomics.

"Sane interfaces" are important. Insane interfaces lead to bugs.

                      Linus

Re: >Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Linus Torvalds]

On Fri, Oct 28, 2011 at 5:19 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> "Sane interfaces" are important. Insane interfaces lead to bugs.

Qutie frankly, if I do "atomic_read()", I do expect to get a single
value. If I don't get a single value, but some mixture of two values,
I'd personally go

  wtf, what does that "atomic" mean in "atomic_read()"?

and I think that's a reasonable wtf to ask.

That said, as mentioned, I don't know of any way to tell gcc "at most once".

Hmm.

Except perhaps using inline asm. Something like this might work:

  static inline int atomic_read(const atomic_t *v)
  {
       int val;
       asm("":"=r" (val):"0" (v->value));
       return val;
  }

(totally untested, but you get the idea: use a non-volatile asm to
make sure that gcc doesn't think it can re-load the value).

That's the trick we use in asmlinkage_protect() and a couple of other
places. It *should* make gcc able to optimize the value away entirely
if it isn't used, but will stop gcc from doing the reload magic.

Does that work for the test-case with VM_BUG_ON()?

                          Linus

Re: >Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Eric Dumazet]

Le vendredi 28 octobre 2011 à 05:40 -0700, Linus Torvalds a écrit :
> On Fri, Oct 28, 2011 at 5:19 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > "Sane interfaces" are important. Insane interfaces lead to bugs.
> 
> Qutie frankly, if I do "atomic_read()", I do expect to get a single
> value. If I don't get a single value, but some mixture of two values,
> I'd personally go
> 
>   wtf, what does that "atomic" mean in "atomic_read()"?
> 
> and I think that's a reasonable wtf to ask.
> 
> That said, as mentioned, I don't know of any way to tell gcc "at most once".
> 
> Hmm.
> 
> Except perhaps using inline asm. Something like this might work:
> 
>   static inline int atomic_read(const atomic_t *v)
>   {
>        int val;
>        asm("":"=r" (val):"0" (v->value));
>        return val;
>   }
> 
> (totally untested, but you get the idea: use a non-volatile asm to
> make sure that gcc doesn't think it can re-load the value).
> 
> That's the trick we use in asmlinkage_protect() and a couple of other
> places. It *should* make gcc able to optimize the value away entirely
> if it isn't used, but will stop gcc from doing the reload magic.
> 
> Does that work for the test-case with VM_BUG_ON()?


On x86 it seems to work :

c050f80f: 0f 84 ea 00 00 00       je     c050f8ff <tcp_sendmsg+0xa1f>
c050f815: 8b 55 bc                mov    -0x44(%ebp),%edx
c050f818: f0 ff 42 10             lock incl 0x10(%edx)      atomic_inc(&page->count)
c050f81c: 8b 02                   mov    (%edx),%eax        page->flags
c050f81e: 25 00 40 02 00          and    $0x24000,%eax
c050f823: 3d 00 40 02 00          cmp    $0x24000,%eax
c050f828: 0f 84 9f 01 00 00       je     c050f9cd <tcp_sendmsg+0xaed>

On x86_64 (gcc-4.6.1 Debian-4.6.1-4) we still have a page->flags useless load,
but the atomic_read() itself is removed.


This looks like a CONFIG_PAGEFLAGS_EXTENDED difference.

In this case, PageTail() is :

#define TESTPAGEFLAG(uname, lname)                  \
static inline int Page##uname(const struct page *page)          \
	{ return test_bit(PG_##lname, &page->flags); } 


So we need a similar idea to remove the volatile from :

static __always_inline int constant_test_bit(unsigned int nr, const volatile unsigned long *addr)
{
return ((1UL << (nr % BITS_PER_LONG)) &
	 (addr[nr / BITS_PER_LONG])) != 0;
}

Re: >Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Linus Torvalds]

On Fri, Oct 28, 2011 at 7:47 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> So we need a similar idea to remove the volatile from :
>
> static __always_inline int constant_test_bit(unsigned int nr, const volatile unsigned long *addr)
> {
> return ((1UL << (nr % BITS_PER_LONG)) &
>         (addr[nr / BITS_PER_LONG])) != 0;

Ho humm. Yeah, I think "test_bit()" falls under the same logic: we do
not want to combine multiple test-bits into one (because we know there
are people looping on it), and we do want to guarantee "access at most
once" semantics, but as with "atomic_read()" we should be able to
optimize away a totally unused test-bit.

The same "empty asm" trick would work there, I think, rather than
using "volatile" (well, the function declaration would still have
"volatile" because it's legal to use a volatile data type, but we'd
cast it away and use the asm trick for the access instead) .

Maybe we should make it a generic helper macro ("ACCESS_AT_MOST_ONCE()")?

Comments? I think I'm open to tested patches..

                      Linus

Re: >Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Eric Dumazet]

Le vendredi 28 octobre 2011 à 07:55 -0700, Linus Torvalds a écrit :

> Maybe we should make it a generic helper macro ("ACCESS_AT_MOST_ONCE()")?
> 
> Comments? I think I'm open to tested patches..
> 

I am currently travelling, but can do that Monday/Tuesday if nobody
beats me ;)

Re: >Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 582 bytes --]

On Fri, Oct 28, 2011 at 7:55 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Comments? I think I'm open to tested patches..

Here's a *untested* patch.

In particular, I worry that I'd need to add a "#include
<linux/compiler.h>" to some header file, although I suspect it gets
included some way regardless.

And when I say "untested", I mean it. I verified that this makes
*some* difference to the generated code, but I didn't actually check
if it really matters, or if it actually compiles and works in general.

Caveat tester,

                                Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1888 bytes --]

 arch/x86/include/asm/bitops.h |    5 +++--
 include/asm-generic/atomic.h  |    2 +-
 include/linux/compiler.h      |    7 +++++++
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
index 1775d6e5920e..e3982cb42fe5 100644
--- a/arch/x86/include/asm/bitops.h
+++ b/arch/x86/include/asm/bitops.h
@@ -308,8 +308,9 @@ static inline int test_and_change_bit(int nr, volatile unsigned long *addr)
 
 static __always_inline int constant_test_bit(unsigned int nr, const volatile unsigned long *addr)
 {
-	return ((1UL << (nr % BITS_PER_LONG)) &
-		(addr[nr / BITS_PER_LONG])) != 0;
+	unsigned long *word = (nr / BITS_PER_LONG) + (unsigned long *)addr;
+	unsigned long bit = 1UL << (nr % BITS_PER_LONG);
+	return (bit & ACCESS_AT_MOST_ONCE(*word)) != 0;
 }
 
 static inline int variable_test_bit(int nr, volatile const unsigned long *addr)
diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h
index e37963c1df4d..c05e21f7a985 100644
--- a/include/asm-generic/atomic.h
+++ b/include/asm-generic/atomic.h
@@ -39,7 +39,7 @@
  * Atomically reads the value of @v.
  */
 #ifndef atomic_read
-#define atomic_read(v)	(*(volatile int *)&(v)->counter)
+#define atomic_read(v)	ACCESS_AT_MOST_ONCE((v)->counter)
 #endif
 
 /**
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 320d6c94ff84..d30ffc685241 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -308,4 +308,11 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
  */
 #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
 
+/*
+ * Like ACCESS_ONCE, but can be optimized away if nothing uses the value,
+ * and/or merged with previous non-ONCE accesses.
+ */
+#define ACCESS_AT_MOST_ONCE(x) \
+	({ typeof(x) __y; asm("":"=r" (__y):"0" (x)); __y; })
+
 #endif /* __LINUX_COMPILER_H */

Re: >Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Eric Dumazet]

Le samedi 29 octobre 2011 à 10:34 -0700, Linus Torvalds a écrit :
> On Fri, Oct 28, 2011 at 7:55 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Comments? I think I'm open to tested patches..
> 
> Here's a *untested* patch.
> 
> In particular, I worry that I'd need to add a "#include
> <linux/compiler.h>" to some header file, although I suspect it gets
> included some way regardless.
> 
> And when I say "untested", I mean it. I verified that this makes
> *some* difference to the generated code, but I didn't actually check
> if it really matters, or if it actually compiles and works in general.
> 
> Caveat tester,
> 

Since jetlag strikes me again, I took your patch and had to change it a
bit, since :

1) x86 uses its own atomic_read() definition in
arch/x86/include/asm/atomic.h

2) We can use a const pointer in ACCESS_AT_MOST_ONCE(*ptr), so I had to
change a bit your implementation, I hope I did not mess it.

Tested (built/booted) on x86 and x86_64

We could logically split this patch in three parts, but hey, maybe I can
try to sleep after all ;)

Thanks

[PATCH] atomic: introduce ACCESS_AT_MOST_ONCE() helper

In commit 4e60c86bd9e (gcc-4.6: mm: fix unused but set warnings)
Andi forced VM_BUG_ON(cond) to evaluate cond, even if CONFIG_DEBUG_VM is
not set :

#ifdef CONFIG_DEBUG_VM
#define VM_BUG_ON(cond) BUG_ON(cond)
#else
#define VM_BUG_ON(cond) do { (void)(cond); } while (0)
#endif

As a side effect, get_page()/put_page_testzero() are performing more bus
transactions on contended cache line on some workloads (tcp_sendmsg()
for example, where a page is acting as a shared buffer)

0,05 :  ffffffff815e4775:       je     ffffffff815e4970 <tcp_sendmsg+0xc80>
0,05 :  ffffffff815e477b:       mov    0x1c(%r9),%eax    // useless                  
3,32 :  ffffffff815e477f:       mov    (%r9),%rax        // useless                  
0,51 :  ffffffff815e4782:       lock incl 0x1c(%r9)                        
3,87 :  ffffffff815e4787:       mov    (%r9),%rax                          
0,00 :  ffffffff815e478a:       test   $0x80,%ah                           
0,00 :  ffffffff815e478d:       jne    ffffffff815e49f2 <tcp_sendmsg+0xd02>      

Thats because both atomic_read() and constant_test_bit() use a volatile
attribute and thus compiler is forced to perform a read, even if the
result is optimized away.

Linus suggested using an asm("") trick and place it in a variant of
ACCESS_ONCE(), allowing compiler to omit reading memory if result is
unused.

This patch introduces ACCESS_AT_MOST_ONCE() helper and use it in the x86
implementation of atomic_read() and constant_test_bit()

on x86_64, we thus reduce vmlinux text a bit (if CONFIG_DEBUG_VM=n)

# size vmlinux.old vmlinux.new
   text	   data	    bss	    dec	    hex	filename
10706848	2894216	1540096	15141160	 e70928	vmlinux.old
10704680	2894216	1540096	15138992	 e700b0	vmlinux.new

Based on a prior patch from Linus

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 arch/x86/include/asm/atomic.h |    2 +-
 arch/x86/include/asm/bitops.h |    7 +++++--
 include/asm-generic/atomic.h  |    2 +-
 include/linux/compiler.h      |   10 ++++++++++
 4 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/atomic.h b/arch/x86/include/asm/atomic.h
index 58cb6d4..b1f0c6b 100644
--- a/arch/x86/include/asm/atomic.h
+++ b/arch/x86/include/asm/atomic.h
@@ -22,7 +22,7 @@
  */
 static inline int atomic_read(const atomic_t *v)
 {
-	return (*(volatile int *)&(v)->counter);
+	return ACCESS_AT_MOST_ONCE(v->counter);
 }
 
 /**
diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
index 1775d6e..e30a190 100644
--- a/arch/x86/include/asm/bitops.h
+++ b/arch/x86/include/asm/bitops.h
@@ -308,8 +308,11 @@ static inline int test_and_change_bit(int nr, volatile unsigned long *addr)
 
 static __always_inline int constant_test_bit(unsigned int nr, const volatile unsigned long *addr)
 {
-	return ((1UL << (nr % BITS_PER_LONG)) &
-		(addr[nr / BITS_PER_LONG])) != 0;
+	const unsigned long *word = (const unsigned long *)addr +
+				    (nr / BITS_PER_LONG);
+	unsigned long bit = 1UL << (nr % BITS_PER_LONG);
+
+	return (bit & ACCESS_AT_MOST_ONCE(*word)) != 0;
 }
 
 static inline int variable_test_bit(int nr, volatile const unsigned long *addr)
diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h
index e37963c..c05e21f 100644
--- a/include/asm-generic/atomic.h
+++ b/include/asm-generic/atomic.h
@@ -39,7 +39,7 @@
  * Atomically reads the value of @v.
  */
 #ifndef atomic_read
-#define atomic_read(v)	(*(volatile int *)&(v)->counter)
+#define atomic_read(v)	ACCESS_AT_MOST_ONCE((v)->counter)
 #endif
 
 /**
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 320d6c9..21f102d 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -308,4 +308,14 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
  */
 #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
 
+/*
+ * Like ACCESS_ONCE, but can be optimized away if nothing uses the value,
+ * and/or merged with previous non-ONCE accesses.
+ */
+#define ACCESS_AT_MOST_ONCE(x)			\
+	({	unsigned long __y;		\
+		asm("":"=r" (__y):"0" (x));	\
+		(__force __typeof__(x)) __y;	\
+	})
+
 #endif /* __LINUX_COMPILER_H */

Re: >Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Andi Kleen]

> +#define ACCESS_AT_MOST_ONCE(x)			\
> +	({	unsigned long __y;		\

why not typeof here?

> +		asm("":"=r" (__y):"0" (x));	\
> +		(__force __typeof__(x)) __y;	\
> +	})
> +

-Andi

Re: >Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Eric Dumazet]

Le dimanche 30 octobre 2011 à 10:59 +0100, Andi Kleen a écrit :
> > +#define ACCESS_AT_MOST_ONCE(x)			\
> > +	({	unsigned long __y;		\
> 
> why not typeof here?
> 
> > +		asm("":"=r" (__y):"0" (x));	\
> > +		(__force __typeof__(x)) __y;	\
> > +	})
> > +
> 
> -Andi

Because it doesnt work if x is const.

/data/src/linux/arch/x86/include/asm/atomic.h: In function
‘atomic_read’:
/data/src/linux/arch/x86/include/asm/atomic.h:25:2: erreur: read-only
variable ‘__y’ used as ‘asm’ output

I understand it wont work for u64 type on 32bit arches, but is
ACCESS_AT_MOST_ONCE() sensible for this kind of usage ?

In this V2, I added a check on sizeof(x) to trigger a compile error.

BTW, I forgot the atomic64_read() possible use of ACCESS_AT_MOST_ONCE()
in arch/x86/include/asm/atomic64_64.h, this saves 600 bytes more :)

On 32bit, I am afraid we cannot change current behavior, because of the
ATOMIC64_ALTERNATIVE() use.

Thanks !

[PATCH] atomic: introduce ACCESS_AT_MOST_ONCE() helper

In commit 4e60c86bd9e (gcc-4.6: mm: fix unused but set warnings)
Andi forced VM_BUG_ON(cond) to evaluate cond, even if CONFIG_DEBUG_VM is
not set :

#ifdef CONFIG_DEBUG_VM
#define VM_BUG_ON(cond) BUG_ON(cond)
#else
#define VM_BUG_ON(cond) do { (void)(cond); } while (0)
#endif

As a side effect, get_page()/put_page_testzero() are performing more bus
transactions on contended cache line on some workloads (tcp_sendmsg()
for example, where a page is acting as a shared buffer)

0,05 :  ffffffff815e4775:       je     ffffffff815e4970 <tcp_sendmsg+0xc80>
0,05 :  ffffffff815e477b:       mov    0x1c(%r9),%eax    // useless
3,32 :  ffffffff815e477f:       mov    (%r9),%rax        // useless
0,51 :  ffffffff815e4782:       lock incl 0x1c(%r9)
3,87 :  ffffffff815e4787:       mov    (%r9),%rax
0,00 :  ffffffff815e478a:       test   $0x80,%ah
0,00 :  ffffffff815e478d:       jne    ffffffff815e49f2 <tcp_sendmsg+0xd02>

Thats because both atomic_read() and constant_test_bit() use a volatile
attribute and thus compiler is forced to perform a read, even if the
result is optimized away.

Linus suggested using an asm("") trick and place it in a variant of
ACCESS_ONCE(), allowing compiler to omit reading memory if result is
unused.

This patch introduces ACCESS_AT_MOST_ONCE() helper and use it in the x86
implementation of atomic_read() and constant_test_bit()

It's also used on x86_64 atomic64_read() implementation.

on x86_64, we thus reduce vmlinux text a bit (if CONFIG_DEBUG_VM=n)

# size vmlinux.old vmlinux.new
   text    data     bss     dec     hex filename
10706848        2894216 1540096 15141160         e70928 vmlinux.old
10704040	2894216	1540096	15138352	 e6fe30	vmlinux.new

Based on a prior patch from Linus, and review from Andi

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
V2: Add a check on sizeof(x) in ACCESS_AT_MOST_ONCE()
    Use ACCESS_AT_MOST_ONCE() on x86_64 atomic64_read()

 arch/x86/include/asm/atomic.h      |    2 +-
 arch/x86/include/asm/atomic64_64.h |    2 +-
 arch/x86/include/asm/bitops.h      |    7 +++++--
 include/asm-generic/atomic.h       |    2 +-
 include/linux/compiler.h           |   15 +++++++++++++++
 5 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/atomic.h b/arch/x86/include/asm/atomic.h
index 58cb6d4..b1f0c6b 100644
--- a/arch/x86/include/asm/atomic.h
+++ b/arch/x86/include/asm/atomic.h
@@ -22,7 +22,7 @@
  */
 static inline int atomic_read(const atomic_t *v)
 {
-	return (*(volatile int *)&(v)->counter);
+	return ACCESS_AT_MOST_ONCE(v->counter);
 }
 
 /**
diff --git a/arch/x86/include/asm/atomic64_64.h b/arch/x86/include/asm/atomic64_64.h
index 0e1cbfc..bdca6fa 100644
--- a/arch/x86/include/asm/atomic64_64.h
+++ b/arch/x86/include/asm/atomic64_64.h
@@ -18,7 +18,7 @@
  */
 static inline long atomic64_read(const atomic64_t *v)
 {
-	return (*(volatile long *)&(v)->counter);
+	return ACCESS_AT_MOST_ONCE(v->counter);
 }
 
 /**
diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
index 1775d6e..e30a190 100644
--- a/arch/x86/include/asm/bitops.h
+++ b/arch/x86/include/asm/bitops.h
@@ -308,8 +308,11 @@ static inline int test_and_change_bit(int nr, volatile unsigned long *addr)
 
 static __always_inline int constant_test_bit(unsigned int nr, const volatile unsigned long *addr)
 {
-	return ((1UL << (nr % BITS_PER_LONG)) &
-		(addr[nr / BITS_PER_LONG])) != 0;
+	const unsigned long *word = (const unsigned long *)addr +
+				    (nr / BITS_PER_LONG);
+	unsigned long bit = 1UL << (nr % BITS_PER_LONG);
+
+	return (bit & ACCESS_AT_MOST_ONCE(*word)) != 0;
 }
 
 static inline int variable_test_bit(int nr, volatile const unsigned long *addr)
diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h
index e37963c..c05e21f 100644
--- a/include/asm-generic/atomic.h
+++ b/include/asm-generic/atomic.h
@@ -39,7 +39,7 @@
  * Atomically reads the value of @v.
  */
 #ifndef atomic_read
-#define atomic_read(v)	(*(volatile int *)&(v)->counter)
+#define atomic_read(v)	ACCESS_AT_MOST_ONCE((v)->counter)
 #endif
 
 /**
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 320d6c9..bd18562 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -308,4 +308,19 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
  */
 #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
 
+#ifndef __ASSEMBLY__
+/*
+ * Like ACCESS_ONCE, but can be optimized away if nothing uses the value,
+ * and/or merged with previous non-ONCE accesses.
+ */
+extern void ACCESS_AT_MOST_ONCE_bad(void);
+#define ACCESS_AT_MOST_ONCE(x)				\
+	({	unsigned long __y;			\
+		if (sizeof(x) > sizeof(__y))		\
+			ACCESS_AT_MOST_ONCE_bad();	\
+		asm("":"=r" (__y):"0" (x));		\
+		(__force __typeof__(x)) __y;		\
+	})
+#endif /* __ASSEMBLY__ */
+
 #endif /* __LINUX_COMPILER_H */

Re: >Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Linus Torvalds]

On Sun, Oct 30, 2011 at 8:16 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> Because it doesnt work if x is const.

Just remove the const. Problem solved.

Both cases of 'const' are totally arbitrary and useless. The
test_bit() one is literally a cast to const (admittedly also *from*
const, but nobody cares), and the atomic_read() one is just because it
uses a silly inline function where a macro would be simpler.

                              Linus

Re: >Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Eric Dumazet]

Le dimanche 30 octobre 2011 à 10:07 -0700, Linus Torvalds a écrit :
> On Sun, Oct 30, 2011 at 8:16 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> > Because it doesnt work if x is const.
> 
> Just remove the const. Problem solved.
> 
> Both cases of 'const' are totally arbitrary and useless. The
> test_bit() one is literally a cast to const (admittedly also *from*
> const, but nobody cares), and the atomic_read() one is just because it
> uses a silly inline function where a macro would be simpler.
> 

Oh well, I am lost. I always considered inline functione better because
of prototype checks.


Changing atomic_read(const atomic_t *v) prototype to
atomic_read(atomic_t *v) is not an option.


To save your time and my time, please select your favorite between :

1) The patch I did

2) 
 static inline int atomic_read(const atomic_t *v)
 {
	return ACCESS_AT_MOST_ONCE(((atomic_t *)v)->counter);
 }

3) 
 static inline int atomic_read(const atomic_t *v)
 {
	return ACCESS_AT_MOST_ONCE(*(int *)&(v)->counter);
 }

4) macro (I personnaly dont like it)
#define atomic_read(v) ACCESS_AT_MOST_ONCE(*(int *)&(v)->counter)

Thanks

Re: >Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Linus Torvalds]

On Sun, Oct 30, 2011 at 10:41 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> Changing atomic_read(const atomic_t *v) prototype to
> atomic_read(atomic_t *v) is not an option.

Why not?

    #define atomic_read(v)     ACCESS_AT_MOST_ONCE((v)->counter)

seems to be the cleanest thing.

And if you don't think this is "an option", I really can't see why you
care about the extra instructions in the code stream either.

> 4) macro (I personnaly dont like it)
> #define atomic_read(v) ACCESS_AT_MOST_ONCE(*(int *)&(v)->counter)

Why the *hell* would you have that cast there?

If somebody passes "const atomic_t"'s around, then just shoot the
bastard. The concept makes no sense.

Grepping for "const atomic_t" shows absolutely *zero* users, except
for the crazy inline function declaration itself.

Stop the insanity already. Get rid of the f*cking "const".

                      Linus

Re: >Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Eric Dumazet]

Le dimanche 30 octobre 2011 à 10:48 -0700, Linus Torvalds a écrit :
> On Sun, Oct 30, 2011 at 10:41 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> > Changing atomic_read(const atomic_t *v) prototype to
> > atomic_read(atomic_t *v) is not an option.
> 
> Why not?
> 
>     #define atomic_read(v)     ACCESS_AT_MOST_ONCE((v)->counter)
> 
> seems to be the cleanest thing.
> 

As I said, because v can be a const pointer provided by the caller.

Try it yourself and you'll discover hundred of call sites doing

.... some_function(const struct *xxx, ...)
{
	if (atomic_read(&xxx->refcnt) <= 0)
		do_something();
	else
		do_otherthing();
}

> And if you don't think this is "an option", I really can't see why you
> care about the extra instructions in the code stream either.
> 

Not an option if we have to change all callers that expected to be able
to use a const atomic_t pointer.

OK, I now have to leave the net.

Re: >Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Linus Torvalds]

On Sun, Oct 30, 2011 at 10:59 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> As I said, because v can be a const pointer provided by the caller.
>
> Try it yourself and you'll discover hundred of call sites doing
>
> .... some_function(const struct *xxx, ...)
> {
>        if (atomic_read(&xxx->refcnt) <= 0)
>                do_something();

Argh. Ok. Testing a refcount in a const struct doesn't make much
sense, but there does seem to be perfectly valid uses of it
(sk_wmem_alloc etc).

Annoying. I guess we have to have those casts. Grr.

                        Linus

Re: >Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Eric Dumazet]

Le dimanche 30 octobre 2011 à 11:09 -0700, Linus Torvalds a écrit :

> Argh. Ok. Testing a refcount in a const struct doesn't make much
> sense, but there does seem to be perfectly valid uses of it
> (sk_wmem_alloc etc).
> 
> Annoying. I guess we have to have those casts. Grr.
> 

OK, please check following patch then.

Thanks !

[PATCH v3] atomic: introduce ACCESS_AT_MOST_ONCE() helper

In commit 4e60c86bd9e (gcc-4.6: mm: fix unused but set warnings)
Andi forced VM_BUG_ON(cond) to evaluate cond, even if CONFIG_DEBUG_VM is
not set :

#ifdef CONFIG_DEBUG_VM
#define VM_BUG_ON(cond) BUG_ON(cond)
#else
#define VM_BUG_ON(cond) do { (void)(cond); } while (0)
#endif

As a side effect, get_page()/put_page_testzero() are performing more bus
transactions on contended cache line on some workloads (tcp_sendmsg()
for example, where a page is acting as a shared buffer)

0,05 :  ffffffff815e4775:    je     ffffffff815e4970 <tcp_sendmsg+0xc80>
0,05 :  ffffffff815e477b:    mov    0x1c(%r9),%eax    // useless
3,32 :  ffffffff815e477f:    mov    (%r9),%rax        // useless
0,51 :  ffffffff815e4782:    lock incl 0x1c(%r9)
3,87 :  ffffffff815e4787:    mov    (%r9),%rax
0,00 :  ffffffff815e478a:    test   $0x80,%ah
0,00 :  ffffffff815e478d:    jne    ffffffff815e49f2 <tcp_sendmsg+0xd02>

Thats because both atomic_read() and constant_test_bit() use a volatile
attribute and thus compiler is forced to perform a read, even if the
result is optimized away.

Linus suggested using an asm("") trick and place it in a variant of
ACCESS_ONCE(), allowing compiler to omit reading memory if result is
unused.

This patch introduces ACCESS_AT_MOST_ONCE() helper and use it in the x86
implementation of atomic_read() and constant_test_bit()

It's also used on x86_64 atomic64_read() implementation.

on x86_64, we thus reduce vmlinux text a bit (if CONFIG_DEBUG_VM=n)

# size vmlinux.old vmlinux.new
   text    data     bss     dec     hex filename
10706848        2894216 1540096 15141160         e70928 vmlinux.old
10704040        2894216 1540096 15138352         e6fe30 vmlinux.new

Basically an extension of a prior patch from Linus

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 arch/x86/include/asm/atomic.h      |    5 +----
 arch/x86/include/asm/atomic64_64.h |    6 ++----
 arch/x86/include/asm/bitops.h      |    6 ++++--
 include/asm-generic/atomic.h       |    2 +-
 include/linux/compiler.h           |   10 ++++++++++
 5 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/atomic.h b/arch/x86/include/asm/atomic.h
index 58cb6d4..2581008 100644
--- a/arch/x86/include/asm/atomic.h
+++ b/arch/x86/include/asm/atomic.h
@@ -20,10 +20,7 @@
  *
  * Atomically reads the value of @v.
  */
-static inline int atomic_read(const atomic_t *v)
-{
-	return (*(volatile int *)&(v)->counter);
-}
+#define atomic_read(v) ACCESS_AT_MOST_ONCE(*(int *)&(v)->counter)
 
 /**
  * atomic_set - set atomic variable
diff --git a/arch/x86/include/asm/atomic64_64.h b/arch/x86/include/asm/atomic64_64.h
index 0e1cbfc..15bbad4 100644
--- a/arch/x86/include/asm/atomic64_64.h
+++ b/arch/x86/include/asm/atomic64_64.h
@@ -1,6 +1,7 @@
 #ifndef _ASM_X86_ATOMIC64_64_H
 #define _ASM_X86_ATOMIC64_64_H
 
+#include <linux/compiler.h>
 #include <linux/types.h>
 #include <asm/alternative.h>
 #include <asm/cmpxchg.h>
@@ -16,10 +17,7 @@
  * Atomically reads the value of @v.
  * Doesn't imply a read memory barrier.
  */
-static inline long atomic64_read(const atomic64_t *v)
-{
-	return (*(volatile long *)&(v)->counter);
-}
+#define atomic64_read(v) ACCESS_AT_MOST_ONCE(*(long *)&(v)->counter);
 
 /**
  * atomic64_set - set atomic64 variable
diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
index 1775d6e..6dcf4b1 100644
--- a/arch/x86/include/asm/bitops.h
+++ b/arch/x86/include/asm/bitops.h
@@ -308,8 +308,10 @@ static inline int test_and_change_bit(int nr, volatile unsigned long *addr)
 
 static __always_inline int constant_test_bit(unsigned int nr, const volatile unsigned long *addr)
 {
-	return ((1UL << (nr % BITS_PER_LONG)) &
-		(addr[nr / BITS_PER_LONG])) != 0;
+	unsigned long *word = (unsigned long *)addr + (nr / BITS_PER_LONG);
+	unsigned long bit = 1UL << (nr % BITS_PER_LONG);
+
+	return (bit & ACCESS_AT_MOST_ONCE(*word)) != 0;
 }
 
 static inline int variable_test_bit(int nr, volatile const unsigned long *addr)
diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h
index e37963c..76ab683 100644
--- a/include/asm-generic/atomic.h
+++ b/include/asm-generic/atomic.h
@@ -39,7 +39,7 @@
  * Atomically reads the value of @v.
  */
 #ifndef atomic_read
-#define atomic_read(v)	(*(volatile int *)&(v)->counter)
+#define atomic_read(v)	ACCESS_AT_MOST_ONCE(*(int *)&(v)->counter)
 #endif
 
 /**
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 320d6c9..307f342 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -308,4 +308,14 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
  */
 #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
 
+/*
+ * Like ACCESS_ONCE, but can be optimized away if nothing uses the value,
+ * and/or merged with previous non-ONCE accesses.
+ */
+#define ACCESS_AT_MOST_ONCE(x)			\
+	({	typeof(x) __y;			\
+		asm("":"=r" (__y):"0" (x));	\
+		__y;				\
+	})
+
 #endif /* __LINUX_COMPILER_H */

Re: >Re: [RFC] should VM_BUG_ON(cond) really evaluate cond

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 567 bytes --]

Hi Linus,

On Sat, 29 Oct 2011 10:34:30 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> In particular, I worry that I'd need to add a "#include
> <linux/compiler.h>" to some header file, although I suspect it gets
> included some way regardless.

Its that assumption that I usually pull people up on ... See
Documentation/SubmitChecklist Rule 1.  It may work for you, but may well
break on some other config or architecture build.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]