1) de-macro, remove ({ usages as side-effect,
2) change calling convention to not accept "flags" by value -- trylock
functions can modify them, so by-value is misleading, and number of users
is relatively low.
3) de-macro spin_trylock_irq() for a change.
Signed-off-by: Alexey Dobriyan <[email protected]>
---
drivers/infiniband/ulp/ipoib/ipoib_main.c | 2 -
drivers/net/atl1e/atl1e_main.c | 2 -
drivers/net/atlx/atl1.c | 2 -
drivers/net/e1000/e1000_main.c | 2 -
drivers/net/e1000e/netdev.c | 2 -
drivers/net/gianfar.c | 2 -
drivers/net/s2io.c | 4 +-
drivers/net/spider_net.c | 2 -
drivers/scsi/aacraid/commsup.c | 4 +-
drivers/serial/uartlite.c | 2 -
include/linux/spinlock.h | 52 ++++++++++++++++++------------
kernel/ptrace.c | 4 +-
kernel/rcupreempt.c | 2 -
kernel/sched.c | 2 -
security/selinux/avc.c | 2 -
15 files changed, 49 insertions(+), 37 deletions(-)
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -711,7 +711,7 @@ static int ipoib_start_xmit(struct sk_buff *skb, struct net_device *dev)
struct ipoib_neigh *neigh;
unsigned long flags;
- if (unlikely(!spin_trylock_irqsave(&priv->tx_lock, flags)))
+ if (unlikely(!spin_trylock_irqsave(&priv->tx_lock, &flags)))
return NETDEV_TX_LOCKED;
if (likely(skb->dst && skb->dst->neighbour)) {
--- a/drivers/net/atl1e/atl1e_main.c
+++ b/drivers/net/atl1e/atl1e_main.c
@@ -1858,7 +1858,7 @@ static int atl1e_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
return NETDEV_TX_OK;
}
tpd_req = atl1e_cal_tdp_req(skb);
- if (!spin_trylock_irqsave(&adapter->tx_lock, flags))
+ if (!spin_trylock_irqsave(&adapter->tx_lock, &flags))
return NETDEV_TX_LOCKED;
if (atl1e_tpd_avail(adapter) < tpd_req) {
--- a/drivers/net/atlx/atl1.c
+++ b/drivers/net/atlx/atl1.c
@@ -2399,7 +2399,7 @@ static int atl1_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
}
}
- if (!spin_trylock_irqsave(&adapter->lock, flags)) {
+ if (!spin_trylock_irqsave(&adapter->lock, &flags)) {
/* Can't get lock - tell upper layer to requeue */
if (netif_msg_tx_queued(adapter))
dev_printk(KERN_DEBUG, &adapter->pdev->dev,
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -3412,7 +3412,7 @@ static int e1000_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
(hw->mac_type == e1000_82573))
e1000_transfer_dhcp_info(adapter, skb);
- if (!spin_trylock_irqsave(&tx_ring->tx_lock, flags))
+ if (!spin_trylock_irqsave(&tx_ring->tx_lock, &flags))
/* Collision - tell upper layer to requeue */
return NETDEV_TX_LOCKED;
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -3614,7 +3614,7 @@ static int e1000_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
if (adapter->hw.mac.tx_pkt_filtering)
e1000_transfer_dhcp_info(adapter, skb);
- if (!spin_trylock_irqsave(&adapter->tx_queue_lock, irq_flags))
+ if (!spin_trylock_irqsave(&adapter->tx_queue_lock, &irq_flags))
/* Collision - tell upper layer to requeue */
return NETDEV_TX_LOCKED;
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -1687,7 +1687,7 @@ static int gfar_poll(struct napi_struct *napi, int budget)
unsigned long flags;
/* If we fail to get the lock, don't bother with the TX BDs */
- if (spin_trylock_irqsave(&priv->txlock, flags)) {
+ if (spin_trylock_irqsave(&priv->txlock, &flags)) {
gfar_clean_tx_ring(dev);
spin_unlock_irqrestore(&priv->txlock, flags);
}
--- a/drivers/net/s2io.c
+++ b/drivers/net/s2io.c
@@ -3083,7 +3083,7 @@ static void tx_intr_handler(struct fifo_info *fifo_data)
unsigned long flags = 0;
u8 err_mask;
- if (!spin_trylock_irqsave(&fifo_data->tx_lock, flags))
+ if (!spin_trylock_irqsave(&fifo_data->tx_lock, &flags))
return;
get_info = fifo_data->tx_curr_get_info;
@@ -4166,7 +4166,7 @@ static int s2io_xmit(struct sk_buff *skb, struct net_device *dev)
if (do_spin_lock)
spin_lock_irqsave(&fifo->tx_lock, flags);
else {
- if (unlikely(!spin_trylock_irqsave(&fifo->tx_lock, flags)))
+ if (unlikely(!spin_trylock_irqsave(&fifo->tx_lock, &flags)))
return NETDEV_TX_LOCKED;
}
--- a/drivers/net/spider_net.c
+++ b/drivers/net/spider_net.c
@@ -530,7 +530,7 @@ spider_net_refill_rx_chain(struct spider_net_card *card)
* and omitting it) is ok. If called by NAPI, we'll be called again
* as spider_net_decode_one_descr is called several times. If some
* interrupt calls us, the NAPI is about to clean up anyway. */
- if (!spin_trylock_irqsave(&chain->lock, flags))
+ if (!spin_trylock_irqsave(&chain->lock, &flags))
return;
while (spider_net_get_descr_status(chain->head->hwdescr) ==
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -1278,7 +1278,7 @@ int aac_reset_adapter(struct aac_dev * aac, int forced)
int retval;
struct Scsi_Host * host;
- if (spin_trylock_irqsave(&aac->fib_lock, flagv) == 0)
+ if (spin_trylock_irqsave(&aac->fib_lock, &flagv) == 0)
return -EBUSY;
if (aac->in_reset) {
@@ -1370,7 +1370,7 @@ int aac_check_health(struct aac_dev * aac)
struct Scsi_Host * host;
/* Extending the scope of fib_lock slightly to protect aac->in_reset */
- if (spin_trylock_irqsave(&aac->fib_lock, flagv) == 0)
+ if (spin_trylock_irqsave(&aac->fib_lock, &flagv) == 0)
return 0;
if (aac->in_reset || !(BlinkLED = aac_adapter_check_health(aac))) {
--- a/drivers/serial/uartlite.c
+++ b/drivers/serial/uartlite.c
@@ -369,7 +369,7 @@ static void ulite_console_write(struct console *co, const char *s,
int locked = 1;
if (oops_in_progress) {
- locked = spin_trylock_irqsave(&port->lock, flags);
+ locked = spin_trylock_irqsave(&port->lock, &flags);
} else
spin_lock_irqsave(&port->lock, flags);
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -320,26 +320,38 @@ do { \
#define spin_trylock_bh(lock) __cond_lock(lock, _spin_trylock_bh(lock))
-#define spin_trylock_irq(lock) \
-({ \
- local_irq_disable(); \
- spin_trylock(lock) ? \
- 1 : ({ local_irq_enable(); 0; }); \
-})
-
-#define spin_trylock_irqsave(lock, flags) \
-({ \
- local_irq_save(flags); \
- spin_trylock(lock) ? \
- 1 : ({ local_irq_restore(flags); 0; }); \
-})
-
-#define write_trylock_irqsave(lock, flags) \
-({ \
- local_irq_save(flags); \
- write_trylock(lock) ? \
- 1 : ({ local_irq_restore(flags); 0; }); \
-})
+static inline int spin_trylock_irq(spinlock_t *lock)
+{
+ local_irq_disable();
+ if (spin_trylock(lock))
+ return 1;
+ else {
+ local_irq_enable();
+ return 0;
+ }
+}
+
+static inline int spin_trylock_irqsave(spinlock_t *lock, unsigned long *flags)
+{
+ local_irq_save(*flags);
+ if (spin_trylock(lock))
+ return 1;
+ else {
+ local_irq_restore(*flags);
+ return 0;
+ }
+}
+
+static inline int write_trylock_irqsave(rwlock_t *lock, unsigned long *flags)
+{
+ local_irq_save(*flags);
+ if (write_trylock(lock))
+ return 1;
+ else {
+ local_irq_restore(*flags);
+ return 0;
+ }
+}
/*
* Pull the atomic_t declaration:
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -174,7 +174,7 @@ repeat:
* cpu's that may have task_lock).
*/
task_lock(task);
- if (!write_trylock_irqsave(&tasklist_lock, flags)) {
+ if (!write_trylock_irqsave(&tasklist_lock, &flags)) {
task_unlock(task);
do {
cpu_relax();
@@ -491,7 +491,7 @@ repeat:
* See ptrace_attach() comments about the locking here.
*/
unsigned long flags;
- if (!write_trylock_irqsave(&tasklist_lock, flags)) {
+ if (!write_trylock_irqsave(&tasklist_lock, &flags)) {
task_unlock(current);
do {
cpu_relax();
--- a/kernel/rcupreempt.c
+++ b/kernel/rcupreempt.c
@@ -867,7 +867,7 @@ static void rcu_try_flip(void)
unsigned long flags;
RCU_TRACE_ME(rcupreempt_trace_try_flip_1);
- if (unlikely(!spin_trylock_irqsave(&rcu_ctrlblk.fliplock, flags))) {
+ if (unlikely(!spin_trylock_irqsave(&rcu_ctrlblk.fliplock, &flags))) {
RCU_TRACE_ME(rcupreempt_trace_try_flip_e1);
return;
}
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -1174,7 +1174,7 @@ static void resched_cpu(int cpu)
struct rq *rq = cpu_rq(cpu);
unsigned long flags;
- if (!spin_trylock_irqsave(&rq->lock, flags))
+ if (!spin_trylock_irqsave(&rq->lock, &flags))
return;
resched_task(cpu_curr(cpu));
spin_unlock_irqrestore(&rq->lock, flags);
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -309,7 +309,7 @@ static inline int avc_reclaim_node(void)
for (try = 0, ecx = 0; try < AVC_CACHE_SLOTS; try++) {
hvalue = atomic_inc_return(&avc_cache.lru_hint) & (AVC_CACHE_SLOTS - 1);
- if (!spin_trylock_irqsave(&avc_cache.slots_lock[hvalue], flags))
+ if (!spin_trylock_irqsave(&avc_cache.slots_lock[hvalue], &flags))
continue;
rcu_read_lock();
On 08/16/2008 11:59 AM, Alexey Dobriyan wrote:
> 1) de-macro, remove ({ usages as side-effect,
> 2) change calling convention to not accept "flags" by value -- trylock
> functions can modify them, so by-value is misleading, and number of users
> is relatively low.
> 3) de-macro spin_trylock_irq() for a change.
Doesn't this break on sparc -- is it tested there? Shouldn't all that be
__always_inline?
> --- a/include/linux/spinlock.h
> +++ b/include/linux/spinlock.h
> @@ -320,26 +320,38 @@ do { \
>
> #define spin_trylock_bh(lock) __cond_lock(lock, _spin_trylock_bh(lock))
>
> -#define spin_trylock_irq(lock) \
> -({ \
> - local_irq_disable(); \
> - spin_trylock(lock) ? \
> - 1 : ({ local_irq_enable(); 0; }); \
> -})
> -
> -#define spin_trylock_irqsave(lock, flags) \
> -({ \
> - local_irq_save(flags); \
> - spin_trylock(lock) ? \
> - 1 : ({ local_irq_restore(flags); 0; }); \
> -})
> -
> -#define write_trylock_irqsave(lock, flags) \
> -({ \
> - local_irq_save(flags); \
> - write_trylock(lock) ? \
> - 1 : ({ local_irq_restore(flags); 0; }); \
> -})
> +static inline int spin_trylock_irq(spinlock_t *lock)
> +{
> + local_irq_disable();
> + if (spin_trylock(lock))
> + return 1;
> + else {
> + local_irq_enable();
> + return 0;
> + }
> +}
> +
> +static inline int spin_trylock_irqsave(spinlock_t *lock, unsigned long *flags)
> +{
> + local_irq_save(*flags);
> + if (spin_trylock(lock))
> + return 1;
> + else {
> + local_irq_restore(*flags);
> + return 0;
> + }
> +}
> +
> +static inline int write_trylock_irqsave(rwlock_t *lock, unsigned long *flags)
> +{
> + local_irq_save(*flags);
> + if (write_trylock(lock))
> + return 1;
> + else {
> + local_irq_restore(*flags);
> + return 0;
> + }
> +}
>
> /*
> * Pull the atomic_t declaration:
* Alexey Dobriyan <[email protected]> wrote:
> 1) de-macro, remove ({ usages as side-effect,
> 2) change calling convention to not accept "flags" by value -- trylock
> functions can modify them, so by-value is misleading, and number of users
> is relatively low.
> 3) de-macro spin_trylock_irq() for a change.
> +++ b/kernel/sched.c
> @@ -1174,7 +1174,7 @@ static void resched_cpu(int cpu)
> struct rq *rq = cpu_rq(cpu);
> unsigned long flags;
>
> - if (!spin_trylock_irqsave(&rq->lock, flags))
> + if (!spin_trylock_irqsave(&rq->lock, &flags))
> return;
hm, i dont really like this assymetric calling convention to other
locking primitives that all take 'flags' as a value.
[spin_lock_irqsave(), etc.]
so what's the point really? It sure does not make actual usage more
readable. If we switched _all_ primitives to use flags as a pointer,
that might make sense, in theory. (but it would also be hugely invasive,
with not much upside with tons of downside like years of migration
fallout and having to rewrite hundreds of kernel hacking books ;-) )
Ingo
On Sat, Aug 16, 2008 at 03:31:00PM +0200, Jiri Slaby wrote:
> On 08/16/2008 11:59 AM, Alexey Dobriyan wrote:
>> 1) de-macro, remove ({ usages as side-effect,
>> 2) change calling convention to not accept "flags" by value -- trylock
>> functions can modify them, so by-value is misleading, and number of
>> users
>> is relatively low.
>> 3) de-macro spin_trylock_irq() for a change.
>
> Doesn't this break on sparc -- is it tested there?
What's so special about sparc?
If you mean compile-tested, yes, compile-tested on
sparc-allnoconfig
sparc-defconfig
sparc-smp-n-debug-n
sparc-smp-n-debug-y
sparc-smp-y-debug-n
sparc-smp-y-debug-y
> Shouldn't all that be __always_inline?
I don't know, likely nobody cares if they are inline or not.
>> --- a/include/linux/spinlock.h
>> +++ b/include/linux/spinlock.h
>> @@ -320,26 +320,38 @@ do { \
>> #define spin_trylock_bh(lock) __cond_lock(lock, _spin_trylock_bh(lock))
>> -#define spin_trylock_irq(lock) \
>> -({ \
>> - local_irq_disable(); \
>> - spin_trylock(lock) ? \
>> - 1 : ({ local_irq_enable(); 0; }); \
>> -})
>> -
>> -#define spin_trylock_irqsave(lock, flags) \
>> -({ \
>> - local_irq_save(flags); \
>> - spin_trylock(lock) ? \
>> - 1 : ({ local_irq_restore(flags); 0; }); \
>> -})
>> -
>> -#define write_trylock_irqsave(lock, flags) \
>> -({ \
>> - local_irq_save(flags); \
>> - write_trylock(lock) ? \
>> - 1 : ({ local_irq_restore(flags); 0; }); \
>> -})
>> +static inline int spin_trylock_irq(spinlock_t *lock)
>> +{
>> + local_irq_disable();
>> + if (spin_trylock(lock))
>> + return 1;
>> + else {
>> + local_irq_enable();
>> + return 0;
>> + }
>> +}
>> +
>> +static inline int spin_trylock_irqsave(spinlock_t *lock, unsigned long
>> *flags)
>> +{
>> + local_irq_save(*flags);
>> + if (spin_trylock(lock))
>> + return 1;
>> + else {
>> + local_irq_restore(*flags);
>> + return 0;
>> + }
>> +}
>> +
>> +static inline int write_trylock_irqsave(rwlock_t *lock, unsigned long
>> *flags)
>> +{
>> + local_irq_save(*flags);
>> + if (write_trylock(lock))
>> + return 1;
>> + else {
>> + local_irq_restore(*flags);
>> + return 0;
>> + }
>> +}
>> /*
>> * Pull the atomic_t declaration:
On Sat, Aug 16, 2008 at 03:46:00PM +0200, Ingo Molnar wrote:
>
> * Alexey Dobriyan <[email protected]> wrote:
>
> > 1) de-macro, remove ({ usages as side-effect,
> > 2) change calling convention to not accept "flags" by value -- trylock
> > functions can modify them, so by-value is misleading, and number of users
> > is relatively low.
> > 3) de-macro spin_trylock_irq() for a change.
>
> > +++ b/kernel/sched.c
> > @@ -1174,7 +1174,7 @@ static void resched_cpu(int cpu)
> > struct rq *rq = cpu_rq(cpu);
> > unsigned long flags;
> >
> > - if (!spin_trylock_irqsave(&rq->lock, flags))
> > + if (!spin_trylock_irqsave(&rq->lock, &flags))
> > return;
>
> hm, i dont really like this assymetric calling convention to other
> locking primitives that all take 'flags' as a value.
> [spin_lock_irqsave(), etc.]
>
> so what's the point really? It sure does not make actual usage more
> readable.
Only slightly, reader is hinted that flags can be changed, otherwise
they will be passed by value.
> If we switched _all_ primitives to use flags as a pointer,
> that might make sense, in theory.
We can't really, and I don't propose that: ~8700 usages of
spin_lock_irqsave, ~1300 usages of local_irq_save. However for code
which has small number of users, why not?
The prehistory of this patch is that I'm deeply in spinlock and
irqflags.h headers for clean irq_flags_t conversion and overall
implession is that they're horrible.
Just the joke with local_irq_enable() defined via raw_local_irq_enable()
and several lines below in the opposite order.
The patch is about slightly cleaner code close to C. ;-)
> (but it would also be hugely invasive,
> with not much upside with tons of downside like years of migration
> fallout and having to rewrite hundreds of kernel hacking books ;-) )
I want my money back for scheduler chapter from "Understanding the Linux Kernel"!
Hi,
Alexey Dobriyan <[email protected]> writes:
> On Sat, Aug 16, 2008 at 03:46:00PM +0200, Ingo Molnar wrote:
>>
>> * Alexey Dobriyan <[email protected]> wrote:
>>
>> > 1) de-macro, remove ({ usages as side-effect,
>> > 2) change calling convention to not accept "flags" by value -- trylock
>> > functions can modify them, so by-value is misleading, and number of users
>> > is relatively low.
>> > 3) de-macro spin_trylock_irq() for a change.
>>
>> > +++ b/kernel/sched.c
>> > @@ -1174,7 +1174,7 @@ static void resched_cpu(int cpu)
>> > struct rq *rq = cpu_rq(cpu);
>> > unsigned long flags;
>> >
>> > - if (!spin_trylock_irqsave(&rq->lock, flags))
>> > + if (!spin_trylock_irqsave(&rq->lock, &flags))
>> > return;
>>
>> hm, i dont really like this assymetric calling convention to other
>> locking primitives that all take 'flags' as a value.
>> [spin_lock_irqsave(), etc.]
>>
>> so what's the point really? It sure does not make actual usage more
>> readable.
>
> Only slightly, reader is hinted that flags can be changed, otherwise
> they will be passed by value.
>
>> If we switched _all_ primitives to use flags as a pointer,
>> that might make sense, in theory.
>
> We can't really, and I don't propose that: ~8700 usages of
> spin_lock_irqsave, ~1300 usages of local_irq_save. However for code
> which has small number of users, why not?
I would also prefer to maintain symmetry here. Your argument is moot,
why diverge a small part of one API just because it is not used much?
Everyone using the spin_lock functions learns the weird interface pretty
fast. If you are in a rare situation where you have to use the trylock
versions, you would really expect them to be used equivalently.
It is weird but diverging it doesn't make it any better.
> The prehistory of this patch is that I'm deeply in spinlock and
> irqflags.h headers for clean irq_flags_t conversion and overall
> implession is that they're horrible.
>
> Just the joke with local_irq_enable() defined via raw_local_irq_enable()
> and several lines below in the opposite order.
>
> The patch is about slightly cleaner code close to C. ;-)
>
>> (but it would also be hugely invasive,
>> with not much upside with tons of downside like years of migration
>> fallout and having to rewrite hundreds of kernel hacking books ;-) )
>
> I want my money back for scheduler chapter from "Understanding the
> Linux Kernel"!
I agree that this argument of Ingo's is not a very good one... ;)
Hannes
On Sun, 17 Aug 2008, Alexey Dobriyan wrote:
> On Sat, Aug 16, 2008 at 03:31:00PM +0200, Jiri Slaby wrote:
> >
> > Doesn't this break on sparc -- is it tested there?
>
> What's so special about sparc?
Sparc _used_ to save/restore the whole processor flags word with the irq
flags. That includes, iirc, things like the register window crap, so if
you did a save/restore flags in a function, it would get all that wrong
and things would blow up.
However, I don't think sparc has actually done that for a _loong_ time
now, due to it always being problematic.
> >> +static inline int spin_trylock_irqsave(spinlock_t *lock, unsigned long *flags)
However, I refuse to see this crap.
Like it or not, the 'flags' argument has always been pass-by-reference,
and not a pointer. It does that because it used to make a huge difference
for gcc code generation and for making it easy to do as a direct inline
asm. We're not going to change that.
Linus
From: Linus Torvalds <[email protected]>
Date: Sat, 16 Aug 2008 14:21:41 -0700 (PDT)
>
>
> On Sun, 17 Aug 2008, Alexey Dobriyan wrote:
>
> > On Sat, Aug 16, 2008 at 03:31:00PM +0200, Jiri Slaby wrote:
> > >
> > > Doesn't this break on sparc -- is it tested there?
> >
> > What's so special about sparc?
>
> Sparc _used_ to save/restore the whole processor flags word with the irq
> flags. That includes, iirc, things like the register window crap, so if
> you did a save/restore flags in a function, it would get all that wrong
> and things would blow up.
>
> However, I don't think sparc has actually done that for a _loong_ time
> now, due to it always being problematic.
We fixed it.
Although I still sometimes consider saving and restoring cpu IRQ flags
in different function contexts to be on the ugly side :)
* Johannes Weiner <[email protected]> wrote:
> >> (but it would also be hugely invasive, with not much upside with
> >> tons of downside like years of migration fallout and having to
> >> rewrite hundreds of kernel hacking books ;-) )
> >
> > I want my money back for scheduler chapter from "Understanding the
> > Linux Kernel"!
>
> I agree that this argument of Ingo's is not a very good one... ;)
i see the smiley, but still - there's a huge difference between the
"pain" caused by a much better scheduler [ hey, did you expect me to say
anything else? ;-) ] and a rather arbitrary value->pointer parametering
change to a core API that is used _everywhere_.
Ingo
Hi Ingo,
Ingo Molnar <[email protected]> writes:
> * Johannes Weiner <[email protected]> wrote:
>
>> >> (but it would also be hugely invasive, with not much upside with
>> >> tons of downside like years of migration fallout and having to
>> >> rewrite hundreds of kernel hacking books ;-) )
>> >
>> > I want my money back for scheduler chapter from "Understanding the
>> > Linux Kernel"!
>>
>> I agree that this argument of Ingo's is not a very good one... ;)
>
> i see the smiley, but still - there's a huge difference between the
> "pain" caused by a much better scheduler [ hey, did you expect me to say
> anything else? ;-) ] and a rather arbitrary value->pointer parametering
> change to a core API that is used _everywhere_.
I just meant the thing about the books. What you said about the
invasive manner, the migration costs and the missing upside I fully
agree with.
Hannes