recursive spinlocks. Shoot.
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 8 lines · In article <[email protected]> you wrote:
Quoted 1 line · Peter T. Breuer
Here's a before-breakfast implementation of a recursive spinlock. That
A looong time ago I gave to someone a recursive spinlock implementation that they integrated in the USB code. I don't see it in the latest kernels, so I have to guess that they found a better solution to do their things. I'm biased to say that it must not be necessary to have the thing if you structure your code correctly.
Well, you can get rid of anything that way. The question is if the interface is an appropriate one to use or not - i.e. if it makes for better code in general, or if it make errors of programming less likely.
I would argue that the latter is undoubtedly true - merely that userspace flock/fcntl works that way would argue for it, but there are a couple of other reasons too.
Going against is the point that it may be slower. Can you dig out your implementation and show me it? I wasn't going for assembler in my hasty example. I just wanted to establish that it's easy, so that it becomes known that its easy, and folks therefore aren't afraid of it. That both you and I have had to write it implies that it's not obvious code to everyone.
Peter
Quoted 19 lines · Peter T. Breuer
Quoted 8 lines · In article <[email protected]> you wrote:
Quoted 1 line · Peter T. Breuer
Here's a before-breakfast implementation of a recursive spinlock. That
A looong time ago I gave to someone a recursive spinlock implementation that they integrated in the USB code. I don't see it in the latest kernels, so I have to guess that they found a better solution to do their things. I'm biased to say that it must not be necessary to have the thing if you structure your code correctly.
Well, you can get rid of anything that way. The question is if the interface is an appropriate one to use or not - i.e. if it makes for better code in general, or if it make errors of programming less likely.
I would argue that the latter is undoubtedly true - merely that userspace flock/fcntl works that way would argue for it, but there are a couple of other reasons too.
Quoted 6 lines
Going against is the point that it may be slower. Can you dig out your implementation and show me it? I wasn't going for assembler in my hasty example. I just wanted to establish that it's easy, so that it becomes known that its easy, and folks therefore aren't afraid of it. That both you and I have had to write it implies that it's not obvious code to everyone.
It was something like this. The real code should be in 2.2.x I believe. The problem at the time was nested ( recursive ) spinlocks.
typedef struct s_nestlock {
spinlock_t lock;
void *uniq;
atomic_t count;
} nestlock_t;#define nestlock_init(snl) \
do { \
spin_lock_init(&(snl)->lock); \
(snl)->uniq = NULL; \
atomic_set(&(snl)->count, 0); \
} while (0)#define nestlock_irqlock(snl, flags) \
do { \
if ((snl)->uniq == current) { \
atomic_inc(&(snl)->count); \
flags = 0; /* No warnings */ \
} else { \
spin_lock_irqsave(&(snl)->lock, flags); \
atomic_inc(&(snl)->count); \
(snl)->uniq = current; \
} \
} while (0)#define nestlock_irqunlock(snl, flags) \
do { \
if (atomic_dec_and_test(&(snl)->count)) { \
(snl)->uniq = NULL; \
spin_unlock_irqrestore(&(snl)->lock, flags); \
} \
} while (0)#define nestlock_lock(snl) \
do { \
if ((snl)->uniq == current) { \
atomic_inc(&(snl)->count); \
} else { \
spin_lock(&(snl)->lock); \
atomic_inc(&(snl)->count); \
(snl)->uniq = current; \
} \
} while (0)#define nestlock_unlock(snl) \
do { \
if (atomic_dec_and_test(&(snl)->count)) { \
(snl)->uniq = NULL; \
spin_unlock(&(snl)->lock); \
} \
} while (0)- Davide
"Davide Libenzi wrote:"
Quoted 9 lines · Peter T. Breuer
Quoted 3 lines
Going against is the point that it may be slower. Can you dig out your implementation and show me it? I wasn't going for assembler in my hasty example. I just wanted to establish that it's easy, so that it becomes
It was something like this. The real code should be in 2.2.x I believe. The problem at the time was nested ( recursive ) spinlocks.
Quoted 5 lines
typedef struct s_nestlock {
spinlock_t lock;
void *uniq;
atomic_t count;
} nestlock_t;This is essentially the same struct as mine. I used the pid of the task, where you use the address of the task. You use an atomic count, whereas I used an ordinary int, guarded by the embedded spinlock.
Quoted 3 lines
#define nestlock_lock(snl) \
do { \
if ((snl)->uniq == current) { \That would be able to read uniq while it is being written by something else (which it can, according to the code below). It needs protection. Probably you want the (< 24 bit) pid instead and to use atomic ops on it.
Quoted 1 line
atomic_inc(&(snl)->count); \
OK, that's the same.
Quoted 4 lines
} else { \
spin_lock(&(snl)->lock); \
atomic_inc(&(snl)->count); \
(snl)->uniq = current; \Hmm .. else we wait for the lock, and then set count and uniq, while somebody else may have entered and be reading it :-). You exit with the embedded lock held, while I used the lock only as a guard to make the ops atomic.
Quoted 8 lines
} \ } while (0)#define nestlock_unlock(snl) \ do { \ if (atomic_dec_and_test(&(snl)->count)) { \ (snl)->uniq = NULL; \ spin_unlock(&(snl)->lock); \
That's OK.
Quoted 2 lines
} \ } while (0)
Well, it's not assembler either, but at least it's easily comparable with the nonrecursive version. It's essentially got an extra if and an inc in the lock. That's all.
I suspect that the rwlock assembler can be coerced to do the job.
Peter
Quoted 11 lines · Peter T. Breuer
This is essentially the same struct as mine. I used the pid of the task, where you use the address of the task. You use an atomic count, whereas I used an ordinary int, guarded by the embedded spinlock.
Quoted 3 lines
#define nestlock_lock(snl) \ do { \ if ((snl)->uniq == current) { \That would be able to read uniq while it is being written by something else (which it can, according to the code below). It needs protection.
No it does not, look better.
Quoted 11 lines
Quoted 1 line
atomic_inc(&(snl)->count); \OK, that's the same.
Quoted 4 lines
} else { \ spin_lock(&(snl)->lock); \ atomic_inc(&(snl)->count); \ (snl)->uniq = current; \Hmm .. else we wait for the lock, and then set count and uniq, while somebody else may have entered and be reading it :-). You exit with
Nope, think about a case were it breaks. False negatives are not possible because it is set by the same task and false positives either.
Quoted 3 lines
Well, it's not assembler either, but at least it's easily comparable with the nonrecursive version. It's essentially got an extra if and an inc in the lock. That's all.
Well, there's a little difference. In case of contention, you loop with your custom try lock while I used the optimized asm code inside spin_lock. But again, I believe we didn't lose anything with the removal of this code (nested/recursive locks) from the kernel.
- Davide
Quoted 10 lines · Peter T. Breuer
Quoted 8 lines · In article <[email protected]> you wrote:
Quoted 1 line · Peter T. Breuer
Here's a before-breakfast implementation of a recursive spinlock. That
A looong time ago I gave to someone a recursive spinlock implementation that they integrated in the USB code. I don't see it in the latest kernels, so I have to guess that they found a better solution to do their things. I'm biased to say that it must not be necessary to have the thing if you structure your code correctly.
Quoted 4 lines
Well, you can get rid of anything that way. The question is if the interface is an appropriate one to use or not - i.e. if it makes for better code in general, or if it make errors of programming less likely.
I dare to disagree. It makes for more messy code in general and might result in the obvious bugs to be replaced by subtle ones that are far harder to debug.
Quoted 3 lines
I would argue that the latter is undoubtedly true - merely that userspace flock/fcntl works that way would argue for it, but there are a couple of other reasons too.
No ;-) Only fcntl does.
Quoted 2 lines
Going against is the point that it may be slower. Can you dig out your implementation and show me it? I wasn't going for assembler in my hasty
It's also a waste of memory. There are many structures that have a lock per instance and four extra bytes (for the owner) would be noticeable. Not that memory is so precious resource, but cachelines may be.
Quoted 4 lines
example. I just wanted to establish that it's easy, so that it becomes known that its easy, and folks therefore aren't afraid of it. That both you and I have had to write it implies that it's not obvious code to everyone.
It's not about weather it's easy. It's about weather it's useful.
-------------------------------------------------------------------------------
Jan 'Bulb' Hudec <[email protected]>