mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: "George Spelvin" <linux@horizon.com>
To: hpa@zytor.com
Cc: linux@horizon.com, linux-kernel@vger.kernel.org,
	schwab@linux-m68k.org, torvalds@linux-foundation.org
Subject: Re: x86-32: clean up rwsem inline asm statements
Date: 13 Jan 2010 14:58:28 -0500	[thread overview]
Message-ID: <20100113195828.2611.qmail@science.horizon.com> (raw)

> As far as I can tell, very few of these assembly statements actually
> need a size at all -- the very first inc statement is purely to memory,
> and as such it needs a size marker, but almost any operation which is
> register-register or register-memory will simply take its size from the
> register operand.  For those, it seems cleaner to simply drop the size
> suffix, and in fact this is the style we have been pushing people
> towards (use the suffix where mandatory or where the size is fixed
> anyway, to help catch bugs; use no suffix where the size can vary and is
> implied by the operands.)

The one thing is that for a register-memory operation, using the
size of the memory operand can catch bugs where it doesn't match
the size of the register operand.

GCC's inline asm doesn't make operand size very implicit, and it's
awkward to cast output operands, so there's a potential for bugs.
I especially get nervous when the operand itself is an immediate
constant, as I can't remember the ANSI rules for the type very well.
(Quick: is 0x80000000 an unsigned 32-bit int or a signed 64-bit one?
What about 2147483648 or 1<<31?)

Since this is mostly inline functions, it's not so big a problem, but
I'd consider something like:

static inline void __up_write(struct rw_semaphore *sem)
{
       unsigned long tmp;
	asm volatile("# beginning __up_write\n\t"
		     LOCK_PREFIX "  xadd%z0	%1,(%2)\n\t"
		     /* tries to transition
			0xffff0001 -> 0x00000000 */
		     "  jz	1f\n"
		     "  call call_rwsem_wake\n"
		     "1:\n\t"
		     "# ending __up_write\n"
       	     : "+m" (sem->count), "=d" (tmp)
       	     : "a" (sem), "1" (-RWSEM_ACTIVE_WRITE_BIAS)
       	     : "memory", "cc");
}

Just in case the size of -RWSEM_ACTIVE_WRITE_BIAS doesn't match that
of sem->count.  It'll explode when you try to run it, of course, but
there's something to be said for compile-time errors.

             reply	other threads:[~2010-01-13 19:58 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-01-13 19:58 George Spelvin [this message]
2010-01-13 21:04 ` H. Peter Anvin
2010-01-14  0:27   ` George Spelvin
2010-01-14  0:39     ` H. Peter Anvin
2010-01-14  1:05   ` Linus Torvalds
  -- strict thread matches above, loose matches on Subject: below --
2010-01-13  0:21 Linus Torvalds
2010-01-13  0:45 ` Andreas Schwab
2010-01-13  1:26   ` Linus Torvalds
2010-01-13  0:48 ` H. Peter Anvin
2010-01-13  0:59   ` Linus Torvalds

Reply instructions:

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

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

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

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

  git send-email \
    --in-reply-to=20100113195828.2611.qmail@science.horizon.com \
    --to=linux@horizon.com \
    --cc=hpa@zytor.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=schwab@linux-m68k.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

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

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

Powered by JetHome