Opened 10 years ago

Last modified 4 months ago

#2657 new enhancement

Implement XSI shared memory <sys/shm.h>

Reported by: kaliber Owned by: emitrax
Priority: normal Milestone: Unscheduled
Component: System/POSIX Version: R1/Development
Keywords: Cc: haiku@…, umccullough@…, richienyhus@…, adek336@…, michaelvoliveira@…, 0xffea@…, mark@…
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

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

Attachments (5)

shm.c (2.7 KB) - added by michaelvoliveira 9 years ago.
code taken from Postgresql-8.1.20
shm.h (1.7 KB) - added by michaelvoliveira 9 years ago.
code taken from postgresql-8.1.20
draft-shm-001.patch (19.3 KB) - added by 0xffea 8 years ago.
draft-shm-002.patch (22.9 KB) - added by 0xffea 8 years ago.
0001-work-in-progress-patch-for-XSI-shm.h-implementation.patch (23.2 KB) - added by korli 5 years ago.
updated patch with style and build fixes

Download all attachments as: .zip

Change History (31)

comment:1 Changed 10 years ago by emitrax

Owner: changed from axeld to emitrax

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

comment:2 Changed 10 years ago by scottmc

Cc: haiku@… added

comment:3 Changed 10 years ago by umccullough

Cc: umccullough@… added

comment:4 Changed 10 years ago by richienyhus

Cc: richienyhus@… added

comment:5 Changed 10 years ago by Adek336

Cc: adek336@… added

comment:6 Changed 9 years ago by kaliber

Component: - GeneralSystem/POSIX

comment:7 Changed 9 years ago by michaelvoliveira

any news?

comment:8 Changed 9 years ago by 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...

comment:9 in reply to:  8 Changed 9 years ago by 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.

comment:10 Changed 9 years ago by michaelvoliveira

Cc: michaelvoliveira@… added

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.

Changed 9 years ago by michaelvoliveira

Attachment: shm.c added

code taken from Postgresql-8.1.20

Changed 9 years ago by michaelvoliveira

Attachment: shm.h added

code taken from postgresql-8.1.20

comment:11 Changed 9 years ago by michaelvoliveira

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

comment:12 Changed 9 years ago by michaelvoliveira

Version: R1/pre-alpha1R1/Development

comment:13 Changed 9 years ago by 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.

comment:14 Changed 8 years ago by 0xffea

Cc: 0xffea@… added

Changed 8 years ago by 0xffea

Attachment: draft-shm-001.patch added

comment:15 Changed 8 years ago by 0xffea

Has a Patch: set

comment:16 Changed 8 years ago by 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.

Changed 8 years ago by 0xffea

Attachment: draft-shm-002.patch added

comment:17 Changed 8 years ago by 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.

comment:18 in reply to:  17 Changed 8 years ago by 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.

Changed 5 years ago by korli

updated patch with style and build fixes

comment:19 Changed 5 years ago by markh

Cc: mark@… added

comment:20 Changed 4 years ago by luroh

Milestone: R1Unscheduled

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

comment:21 Changed 3 years ago by michaelvoliveira

Ping.

Has an updated patch here waiting so long for a commit

comment:22 in reply to:  21 Changed 3 years ago by 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.

comment:23 Changed 3 years ago by michaelvoliveira

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

comment:24 Changed 12 months ago by pulkomandy

Has a Patch: unset

comment:25 Changed 12 months ago by pulkomandy

Has a Patch: unset

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

comment:26 Changed 4 months ago by waddlesplash

Has a Patch: unset

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?

Note: See TracTickets for help on using tickets.