[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] ipc: perform conditional locking while changing the port seq
Re: [PATCH] ipc: perform conditional locking while changing the port sequence number
Sat, 7 Sep 2013 10:46:18 +0200
Mutt/1.5.21+34 (58baf7c9f32f) (2010-12-30)
Marin Ramesa, le Sat 07 Sep 2013 10:17:29 +0200, a écrit :
> On 07.09.2013 08:58:14, Samuel Thibault wrote:
> > Marin Ramesa, le Sat 07 Sep 2013 08:00:47 +0200, a écrit :
> > > * ipc/ipc_port.c (ipc_port_set_seqno) [MACH_SLOCKS]: Conditional
> > > locking.
> > What is the rationale? Does it really bring an noticeable
> > improvement? The locking is already conditional inside
> > ipc_port_lock_mqueue, from the simple_*lock* macros themselves. That
> > is way more readable than having ifdefs inside the main source code.
> That code in ipc_port_set_seqno() is simply not functional (except the
> change in sequence number) in the case when MACH_SLOCKS is not defined.
What do you mean by "not functional"? Did you actually notice a bug?
That's the first thing a rational should mention.
> We have a local variable mqueue that is set by the lock, but if you
> look at the definition of imq_unlock() and then simple_unlock() members
> of mqueue just change state and they don't have any effect on any other
Sure, that is what usually happens for a locking/unlocking primitive
pair: its only purpose is to change the state of the lock, and the side
effect is obtained simply because other portions of the code use the
same lock (imq_lock), to protect the same thing (the seqno).
> plus it's a local variable - I would understand the code if
> mqueue is more global, but it's not.
The local variable is simply used to store the queue associated with the
port, in order to be able to unlock it.
> And in the end mqueue never get's really used in the
It is, for locking.
> I tried to go around this by using MACH_SLOCKS and just change the
> sequence number otherwise. But if you think it's a bad idea maybe
> there's another way.
Maintainability of the code is usually a very important thing, so it's
probably better to find another way to fix the bug, if any.