Subject: Re: Dynamic limits for SysV semaphores and message queues
To: Mindaugas R. <rmind@NetBSD.org>
From: Greg Troxel <firstname.lastname@example.org>
Date: 10/07/2007 10:22:09
"Mindaugas R." <rmind@NetBSD.org> writes:
> Greg Troxel <email@example.com> wrote:
>> You have hoisted the special case out of the loops, and I think in one
>> case this introduced a bug: msghdrs[msginfo.msgtql-1].msg_type doesn't
>> get set to 0. I'd prefer to leave the loops whole and use the if in
>> these cases, as it's clearer to verify correctness. I think your change
>> to make the top element the special case one helps understandability.
> Actually, it does not matter since the memory is zeroed. But yes, I will set
> msg_type for the readability. However, moving such special cases out of the
> loop where appropriate makes it more readable for me.
It's fine with me; I just like it to be easy for a reviewer to be sure,
even at the expense of lines that could be omitted.
>> Does the new_msgpool = ptr + size idiom adequately ensure alignment? If
>> it does because of the structure contents, a comment explaining why
>> would be nice to perhaps avoid future problems. Or a KASSERT about
>> size/alignment. (Did you try this on sparc64?)
> I am not sure what do you mean. Do you mean a correct counting of offsets?
> In such case - yes. Also, take a look at msginit() or seminit().
The code mallocs a large area that is the sum of sizes, and then uses
the sizes to grab sub-areas. This does not guarantee that all of the
sub-areas will have appropriate alignment for their data structures.
For example, if the second area were char foo, the third area
wouldn't even be byte aligned. I don't know what's in all the structs,
and they're probably all 8-byte aligned, but this seems in general
> Re sparc64: No. Volunteer to test? :)
Sorry, I don't have one set up that I can test on right now - both of
mine that are running are in production with release branches.
>> I don't see how the if(msg_realloc_state) (at 455/659) interacts with
>> reallocation, because it looks like msgmutex takes care of this.
> In case there are waiters, msgrealloc() might sleep in cv_wait(). At that
> moment, another msgrcv() call might happen which waits on cv_wait_sig().
> This check disallows such situation.
I'm willing to take your word for it; I'm a fan of more comments about
things that are hard to understand - but I haven't studied the new cv_*
regime at all, and arguably knowing that is a prereq for being a proper