Re: PATCH: Allow shared semaphores to be really shared
Date: Sat, 21 Dec 2013 02:29:49 -0500
From: Ted Unangst t...@tedunangst.com
On Sat, Dec 07, 2013 at 14:11, Vadim Zhukov wrote:
This patch fixes problems in KDE4, that relies on sharing (process-shared)
semaphores via mmap(). This feature is used in the wild, so if we claim
that we support process-shared semaphores, we have to implement it, too.
Haven't forgotten about this.
Dirty games with lock member required to avoid exposing extra structure
(struct _spinlock) in public header. On the bright side, this diff removes
more stuff than it adds, so it have to be a good diff. :)
This is really the troubling part for me. In particular, our spinlock
implementation isn't all that great. I was poking at it earlier this
year, but didn't make any progress except to convince myself more work
needs to be done. Exposing spinlock internals makes experimentation in
this area more difficult.
I realized my previous replies had only vague promises of pain and
misery resulting from changing sem_t :), but I've been thinking about
it a bit more and wanted to make clear that there's a concrete reason
behind my concern. Unfortunately, there's no timeframe for when I
expect this to happen, other than I consider spinlock/mutex
performance a serious issue.
So yes, I'm stalling/slacking, but I hope you'll find the patience to
wait it out. And if not, let's talk.
It's worth looking at what Linux and Solaris have done here. Linux
only exposes a semi-opaque struct to userland:
typedef union
{
char __size[__SIZEOF_SEM_T];
long int __align;
} sem_t;
It's actually a union, such that you can add a member that forces
alignment, but still specify the size easily in bytes. Might be
better to use 'long long' as the alignment type here though.
If you look at Solaris, you'll realize that they realized too late
that they should have done the same. There only parts of the struct
are opaque.
As Ted pointed out, the size of the struct (or union) becomes part of
the ABI. So we should probably reserve a bit more space than we
actually need for the implementation. That way, we'll leave some room
for experimentation. Wasting a bit more memory shouldn't be a big
issue as typical code that uses semaphores won't be constructing
thousands of these. And rounding up to the nearest power of two
should be pretty much free.
This will cause some ugliness in the actual implementation as we'll
have to cast this semi-opaque struct to the real type there. And we
probably should add a compile-time assert in there to make sure the
real-type isn't bugger than the type we expose to userland.
Re: PATCH: Allow shared semaphores to be really shared
On Sat, Dec 21, 2013 at 12:42, Mark Kettenis wrote: As Ted pointed out, the size of the struct (or union) becomes part of the ABI. So we should probably reserve a bit more space than we actually need for the implementation. That way, we'll leave some room for experimentation. Wasting a bit more memory shouldn't be a big issue as typical code that uses semaphores won't be constructing thousands of these. And rounding up to the nearest power of two should be pretty much free. Something I've also belatedly realized is we are in effect already exposing internals via sem_open. Two programs with two different majors of libpthread.so can open the same named semaphore, but it may be different internally. We should add a version number inside the semaphore to prevent that from happening.
Re: PATCH: Allow shared semaphores to be really shared
On Sat, Dec 07, 2013 at 14:11, Vadim Zhukov wrote: This patch fixes problems in KDE4, that relies on sharing (process-shared) semaphores via mmap(). This feature is used in the wild, so if we claim that we support process-shared semaphores, we have to implement it, too. Haven't forgotten about this. Dirty games with lock member required to avoid exposing extra structure (struct _spinlock) in public header. On the bright side, this diff removes more stuff than it adds, so it have to be a good diff. :) This is really the troubling part for me. In particular, our spinlock implementation isn't all that great. I was poking at it earlier this year, but didn't make any progress except to convince myself more work needs to be done. Exposing spinlock internals makes experimentation in this area more difficult. I realized my previous replies had only vague promises of pain and misery resulting from changing sem_t :), but I've been thinking about it a bit more and wanted to make clear that there's a concrete reason behind my concern. Unfortunately, there's no timeframe for when I expect this to happen, other than I consider spinlock/mutex performance a serious issue. So yes, I'm stalling/slacking, but I hope you'll find the patience to wait it out. And if not, let's talk.
Re: PATCH: Allow shared semaphores to be really shared
On Wed, Dec 11, 2013 at 11:12, Vadim Zhukov wrote: If we go back to returning ENOMEM or whatever in sem_init, does that fix KDE? If we stop pretending we support shared unnamed semaphores, then, yes, this will help KDE. But I cannot gurantee there will be no other fallout. This needs a deep scan of all ports source as minimum... I'd rather disable shared semaphores totally, until all issues are fixed, then. I don't see a point in having half-working stuff in tree which gets picked up and used. Maybe I wasn't clear, that's exactly what I wanted to do. Anyway, I just went ahead and did it. sem_init(pshared) will return EPERM as before.
Re: PATCH: Allow shared semaphores to be really shared
2013/12/9 Ted Unangst t...@tedunangst.com: On Mon, Dec 09, 2013 at 19:49, Vadim Zhukov wrote: So what's the decision? Are there any objections still? If not, can I have a pair of okays? KDE4 really needs a decision to be made: people already had apps crashing without this diff, so I've put a dirty hack to stop KDE using of process-shared semaphores. But there could be more dragons, in other software. If we go back to returning ENOMEM or whatever in sem_init, does that fix KDE? If we stop pretending we support shared unnamed semaphores, then, yes, this will help KDE. But I cannot gurantee there will be no other fallout. This needs a deep scan of all ports source as minimum... I don't mean to be a dick about this, but in general here's how we do things. If a feature is added and it introduces a regression, we revert. We don't dig the hole deeper by committing an even more intrusive diff. I'm agree, this is a good practice. I'm not saying the diff is bad or can't be made to work, but I don't like using KDE needs this now to rush it in if there are other options. I also don't think the thing you're doing with the spinlock is something we want. This was done to avoid polluting exported symbols. It's ugly, yes. I do not see any better option that doesn't involve rewriting entire locking in librthread, though; I hope that I just miss something obvious. I want time to get that right. I'd rather disable shared semaphores totally, until all issues are fixed, then. I don't see a point in having half-working stuff in tree which gets picked up and used. Yes, it was my bad too that I didn't thought about sharing semaphores via mapped memory in the first place. -- WBR, Vadim Zhukov
Re: PATCH: Allow shared semaphores to be really shared
2013/12/8 Philip Guenther guent...@gmail.com: On Sat, Dec 7, 2013 at 10:35 AM, Ted Unangst t...@tedunangst.com wrote: One of the hallmarks of the original libpthread was that all data structures were opaque, and hidden via pointers. That in turn made it possible to write a binary compatible librthread. I never would have started librthread if it hadn't been for that compatibility. So I don't like turning my back on it. Exposing the size of sem_t is a *major* step. It may be time to actually make that jump. sem_t is actually an easy one to do it with, as it doesn't have the ownership issues of mutexes or spinlocks, or the lists of condvars or barriers. Those others will require more structure changes and kernel help. So what's the decision? Are there any objections still? If not, can I have a pair of okays? KDE4 really needs a decision to be made: people already had apps crashing without this diff, so I've put a dirty hack to stop KDE using of process-shared semaphores. But there could be more dragons, in other software. -- WBR, Vadim Zhukov
Re: PATCH: Allow shared semaphores to be really shared
On Mon, Dec 09, 2013 at 19:49, Vadim Zhukov wrote: So what's the decision? Are there any objections still? If not, can I have a pair of okays? KDE4 really needs a decision to be made: people already had apps crashing without this diff, so I've put a dirty hack to stop KDE using of process-shared semaphores. But there could be more dragons, in other software. If we go back to returning ENOMEM or whatever in sem_init, does that fix KDE? I don't mean to be a dick about this, but in general here's how we do things. If a feature is added and it introduces a regression, we revert. We don't dig the hole deeper by committing an even more intrusive diff. I'm not saying the diff is bad or can't be made to work, but I don't like using KDE needs this now to rush it in if there are other options. I also don't think the thing you're doing with the spinlock is something we want. I want time to get that right.
Re: PATCH: Allow shared semaphores to be really shared
On Sat, Dec 07, 2013 at 14:11, Vadim Zhukov wrote: This patch fixes problems in KDE4, that relies on sharing (process-shared) semaphores via mmap(). This feature is used in the wild, so if we claim that we support process-shared semaphores, we have to implement it, too. This changes the sem_t definition: it becomes a synonim to struct __sem instead of being a pointer to it. This way we allow apps to have sem_t in shared memory via sem_init() and thus share it between processes (by, e.g., mmap()'ing the same file). Of course, this forces a major library version bump. This requires more thought. If the introduction of pshared support is breaking things, maybe just that part can be reverted for now.
Re: PATCH: Allow shared semaphores to be really shared
2013/12/7 Ted Unangst t...@tedunangst.com: On Sat, Dec 07, 2013 at 14:11, Vadim Zhukov wrote: This patch fixes problems in KDE4, that relies on sharing (process-shared) semaphores via mmap(). This feature is used in the wild, so if we claim that we support process-shared semaphores, we have to implement it, too. This changes the sem_t definition: it becomes a synonim to struct __sem instead of being a pointer to it. This way we allow apps to have sem_t in shared memory via sem_init() and thus share it between processes (by, e.g., mmap()'ing the same file). Of course, this forces a major library version bump. This requires more thought. If the introduction of pshared support is breaking things, maybe just that part can be reverted for now. Well, it's the half-done introduction of pshared support is breaking things. I do not see any fallout now that sharing unnamed semaphores works. But what we'll gain with revert? Except that it's still better than current situation. -- WBR, Vadim Zhukov
Re: PATCH: Allow shared semaphores to be really shared
On Sat, Dec 07, 2013 at 21:32, Vadim Zhukov wrote: 2013/12/7 Ted Unangst t...@tedunangst.com: On Sat, Dec 07, 2013 at 14:11, Vadim Zhukov wrote: This patch fixes problems in KDE4, that relies on sharing (process-shared) semaphores via mmap(). This feature is used in the wild, so if we claim that we support process-shared semaphores, we have to implement it, too. This changes the sem_t definition: it becomes a synonim to struct __sem instead of being a pointer to it. This way we allow apps to have sem_t in shared memory via sem_init() and thus share it between processes (by, e.g., mmap()'ing the same file). Of course, this forces a major library version bump. This requires more thought. If the introduction of pshared support is breaking things, maybe just that part can be reverted for now. Well, it's the half-done introduction of pshared support is breaking things. I do not see any fallout now that sharing unnamed semaphores works. But what we'll gain with revert? Except that it's still better than current situation. One of the hallmarks of the original libpthread was that all data structures were opaque, and hidden via pointers. That in turn made it possible to write a binary compatible librthread. I never would have started librthread if it hadn't been for that compatibility. So I don't like turning my back on it. Exposing the size of sem_t is a *major* step.
Re: PATCH: Allow shared semaphores to be really shared
07.12.2013 22:35 пользователь Ted Unangst t...@tedunangst.com написал: On Sat, Dec 07, 2013 at 21:32, Vadim Zhukov wrote: 2013/12/7 Ted Unangst t...@tedunangst.com: On Sat, Dec 07, 2013 at 14:11, Vadim Zhukov wrote: This patch fixes problems in KDE4, that relies on sharing (process-shared) semaphores via mmap(). This feature is used in the wild, so if we claim that we support process-shared semaphores, we have to implement it, too. This changes the sem_t definition: it becomes a synonim to struct __sem instead of being a pointer to it. This way we allow apps to have sem_t in shared memory via sem_init() and thus share it between processes (by, e.g., mmap()'ing the same file). Of course, this forces a major library version bump. This requires more thought. If the introduction of pshared support is breaking things, maybe just that part can be reverted for now. Well, it's the half-done introduction of pshared support is breaking things. I do not see any fallout now that sharing unnamed semaphores works. But what we'll gain with revert? Except that it's still better than current situation. One of the hallmarks of the original libpthread was that all data structures were opaque, and hidden via pointers. That in turn made it possible to write a binary compatible librthread. I never would have started librthread if it hadn't been for that compatibility. So I don't like turning my back on it. Exposing the size of sem_t is a *major* step. Then we have to state that we won't support process-shared semaphores, as well as mutexes and other locks, ever. Because all of them should have ability to live in shared memory. The only other way to achieve process-shared locks I see, is to use sem_t and others as indexes in some kernel structures (like file descriptors), moving most of librthread into the kernel. That sounds too crazy even for me. :) I like opaque types of librthread, too. And I hate what I did with one of them already. Too bad for us that the world does not care. :(
Re: PATCH: Allow shared semaphores to be really shared
On Sat, Dec 7, 2013 at 10:35 AM, Ted Unangst t...@tedunangst.com wrote: One of the hallmarks of the original libpthread was that all data structures were opaque, and hidden via pointers. That in turn made it possible to write a binary compatible librthread. I never would have started librthread if it hadn't been for that compatibility. So I don't like turning my back on it. Exposing the size of sem_t is a *major* step. It may be time to actually make that jump. sem_t is actually an easy one to do it with, as it doesn't have the ownership issues of mutexes or spinlocks, or the lists of condvars or barriers. Those others will require more structure changes and kernel help. Philip Guenther
