[haiku-bugs] Re: [Haiku] #2657: Implement XSI shared memory <sys/shm.h>

#2657: Implement XSI shared memory <sys/shm.h>
---------------------------+------------------------------
  Reporter:  kaliber       |        Owner:  emitrax
      Type:  enhancement   |       Status:  new
  Priority:  normal        |    Milestone:  R1
 Component:  System/POSIX  |      Version:  R1/Development
Resolution:                |     Keywords:
Blocked By:                |  Has a Patch:  1
  Platform:  All           |     Blocking:
---------------------------+------------------------------

Comment (by bonefish):

 Replying to [comment:17 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 `VMCache`s 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.

-- 
Ticket URL: <http://dev.haiku-os.org/ticket/2657#comment:18>
Haiku <http://dev.haiku-os.org>
Haiku - the operating system.

Other related posts: