Implement XSI shared memory <sys/shm.h>

[kaliber]

sys/shm.h is missing and not implemented in Haiku. It would be nice to have it.

​http://www.opengroup.org/onlinepubs/009695399/basedefs/sys/shm.h.html

[emitrax]

If no one else wants it, I should be able to do it.

[michaelvoliveira]

any news?

[michaelvoliveira]

This one will be necessary for a decent port of boost-1.42

We could use this one from FreeBSD.. of course, since that we axe some functions from there...

[bonefish]

Replying to michaelvoliveira:

This one will be necessary for a decent port of boost-1.42

We could use this one from FreeBSD.. of course, since that we axe some functions from there...

The header is the trivial part (copy and paste from the specs). The implementation is the actual work.

[michaelvoliveira]

Ok them...

This code (BSD 3-clause) has taken from postgresql-8.1.20..

But I don't know how make a good patch because I don't know where shm.c would fit in the tree.

[michaelvoliveira]

code taken from Postgresql-8.1.20

[michaelvoliveira]

code taken from postgresql-8.1.20

[michaelvoliveira]

The code builds fine under gcc2 and gcc4 without warnings...

[bonefish]

Besides only approximating the feature (one can't emulate it correctly with areas), the code has other problems, like missing error checks, incorrect return values and incorrect function signatures.

[bonefish]

Thanks for the patch. I haven't looked too closely yet, but it seems a good start. There's a whole bunch of coding style issues, though. Just pointing out a few:

• Two blank lines between groups of unrelated stuff -- e.g. before and after #include, #define, variable declaration, etc. blocks and between functions. Never within functions or class definitions, though (max. one blank line there). No blank line between copyright header and header guard.
• No blank line at the beginning or end of blocks (SharedMemoryEntry::HasPermission()).
• Don't indent/align with anything but tabs. Only exception is in class definitions when e.g. the type needs more space than there is in its column. Then a single space can be used to separate it from the next column.
• Don't indent names in local variable definitions.
• No spaces after C cast operator (wrong: (int) fArea).
• No space between function name and opening parenthesis (also new(std::nothrow)), but single space between if/switch/... and opening parenthesis.
• Always name parameters in function/method prototypes.
• Don't use uncommon abbreviations in identifiers. E.g. addr should be address, fDs should be fData or fDefinition or whatever that might stand for.
• Use a consistent pointer style, preferably Foo* foo.
• No unnecessary parentheses (wrong: fDs.shm_perm.mode = (flags & 0x01ff);).
• In function definitions define variables where they are first used, not at the beginning of the function. Whenever possible combine definition and initialization (status_t status = ...).
• if (fArea < B_OK): Compare with < 0 instead, when values >= 0 are valid. Use the stricter comparison != B_OK, when the value is supposed to be an error code (i.e. B_OK or another error code, normally of type status_t).
• Explicitly force bit check expressions to boolean ((flags & IPC_CREAT) != 0).
• When wrapping an expression, the operator always goes to beginning of the next line.
• Broken indentation in SharedMemoryEntry::HasReadPermission() and _user_xsi_shmget().

I haven't really looked at the implementation, but I noticed:

• _user_xsi_shmat(): raddr needs to be copied out via user_memcpy(). Besides, it needs a better name. Also, it is recommended to prefix output parameters with _, e.g. _mappedAddress in this case.
• Using fully locked kernel areas is not a good idea. The locking isn't necessary at all. But the bigger issue is that kernel address space is a rather limited resource. I'd only create a VMCache and map it into the user address spaces as needed, but not into the kernel address space at all.

[0xffea]

bonefish: thanks for the very helpful comments!

i tried to correct the style issues and not to use areas in kernel space.

• create a cache with VMCacheFactory::CreateAnonymousCache
• use VMAddressSpace->CreateArea and VMAddressSpace->InsertArea to create the area in user space

hope this is the right way to do it. not sure about the proper locking.

[bonefish]

Replying to 0xffea:

i tried to correct the style issues and not to use areas in kernel space.

The coding style looks much better, now. A few issues remain:

• Inconsistent parameter naming in prototypes/implementations of various related functions:
  • *shmctl(): mds, memoryDs, _buffer, buffer. Unless you come up with a better fitting name, buffer is the best one so far.
  • *shmat(): memoryPointer -> address, rAddress -> _returnAddress
  • *shmdt(): memoryPointer -> address
• Inconsistent pointer style in the _kern_*() prototypes. Since both styles are used in the file, either would be OK, but mixing in one block or even one function is not so nice. Preferred is Foo* foo.
• In syscalls.h: struct shmid_ds is in wrong line.
• In xsi_shared_memory.cpp: The <kernel/...> includes block is not sorted and the kernel/ part should be omitted. The block should be joined with the <vm/...> and <util/...> includes blocks (both are kernel private includes).
• MAX_XSI_SHARED_MEMORY_SIZE definition: Missing spaces between the operators and operands. Also, the whole expression should be put in parentheses to prevent potential operator precedence issues where the macro is expanded.
• In both *HashTableDefinition classes:
  • Incorrect space after HashKey.
  • Superfluous const of parameter type in HashKey() and Compare().
  • Superfluous parenthesis in HashKey() implementation.
• There are still a few instances of implicit (foo & bar) to bool conversions. Also, !(flags & IPC_CREAT) should be (flags & IPC_CREAT) == 0.
• Superfluous parenthesis in if ((ipcKey->Size() < size) && (size != 0)) (_user_xsi_shmget()), besides that, due to size_t being unsigned, the second check is superfluous.
• Incorrect (void*) cast spacing in _user_xsi_shmat() (2x).
• Inconsistent pointer style in xsi_shm.cpp.

• create a cache with VMCacheFactory::CreateAnonymousCache
• use VMAddressSpace->CreateArea and VMAddressSpace->InsertArea to create the area in user space

hope this is the right way to do it. not sure about the proper locking.

Unfortunately things are quite a bit more complicated:

• A VMAddressSpace object is reference counted and has a read/write lock. Holding the write lock is required for any manipulation operation (creating, deleting, resizing areas) and also for manipulating most members of the VMArea objects contained in the address space. Holding the read lock guarantees that the respective address space and area members don't change.
• A VMCache object is reference counted and has a simple mutex lock. The lock must be held for almost all kinds of accesses to the object.
• Locking order is: address space -> area cache -> source cache -> .... If multiple address spaces need to be locked, a MultiAddressSpaceLocker must be used. There are more locking helper classes in src/system/kernel/vm/VMAddressSpaceLocking.h.
• To map a VMCache into an address space, creating an area, the function of choice is map_backing_store() in vm/vm.cpp. It gets the job done with all the bells and whistles (including address and alignment constraints, unmapping of previously mapped stuff, etc.). Only problem, it's currently local to that source file. A simple kernel-private wrapper (e.g. vm_map_cache()) could be introduced (prototype in <vm/vm.h>).
• When creating an area, it must be marked B_SHARED_AREA or it will have copy-on-write behavior on fork().

Generally I would recommend having a look at vm/vm.cpp. There are quite a few functions that implement functionality similar to what you need. E.g. vm_create_anonymous_area() creates a cache and maps it.

I haven't looked closely at the features the POSIX API requires, but it might be possible to avoid playing with VMCaches altogether by using memory-mapped files. That would allow for using a higher-lever API, probably simplifying things quite a bit.

Regarding the tests: Actually using the shared memory is missing and is definitely something that should be tested, also with multiple processes. I believe the OpenPosixTestsuite in src/tests/system/libroot/posix/posixtestsuite also has XSI shared memory tests. Unfortunately some tests of the testing framework use functionality Haiku is lacking or doesn't implement correctly yet (clock_*(), signals), so the respective tests might not pass anyway, but it certainly doesn't harm to have a look.

[korli]

updated patch with style and build fixes

[luroh]

Move POSIX compatibility related tickets out of R1 milestone (FutureHaiku/Features).

[michaelvoliveira]

Ping.

Has an updated patch here waiting so long for a commit

[bonefish]

Replying to michaelvoliveira:

Has an updated patch here waiting so long for a commit

I haven't looked at the latest patch, but if korli has only addressed the style issues and applied build fixes as its description says, the second part of comment:18 still remains to be addressed.

[michaelvoliveira]

Thanks for the reply! I'll try build libboost again using #ifdef and vm_create_anonymous_area() like you suggested.

[pulkomandy]

Patch migrated to Gerrit: ​https://review.haiku-os.org/#/c/77/

[waddlesplash]

Besides only approximating the feature (one can't emulate it correctly with areas),

@bonefish, how so? It looks like we would need a set of housekeeping structs or the like to keep track of the "modification time", etc. and other such values, so we would still need new syscalls, but AFAICT there is nothing that shm areas can do that Be areas cannot -- so can you perhaps elaborate on what those are, if you can recall?