Closed Bug 523172 Opened 10 years ago Closed 7 years ago

Add a cross-platform, low-level mozilla::ipc::SharedMemory class.

Categories

(Core :: IPC, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: cjones, Assigned: rohitssingh)

References

Details

Attachments

(1 file, 2 obsolete files)

This class should be a minimal wrapper around the shmem primitives provided by POSIX and win32.  I suggest that this be structured as an ipc/glue/SharedMemory.h header with ipc/glue/SharedMemory_posix.cc and ipc/glue/SharedMemory_win32.cc implementations.  (We may later want to specialize SharedMemory_posix for mac.)
To be clearer, I'd like this class to provide all the methods that the chromium base::SharedMemory class provides, except for: GiveToProcess(), Lock(), and Unlock().  We can discuss specific method signatures during review.

I'd also like mozilla::ipc::SharedMemory to implement two more methods:

  static size_t SystemPageSize(); 

which returns what you would expect it to, and

  enum Rights { RightNone = 0, RightRead = 1 << 0, RightWrite = 1 << 1 };
  void Protect(Rights, const void*, size_t);

That way clients can call |shmem.Protect(RightRead | RightWrite, ...)| to set access controls for particular pages.  On POSIX systems this should wrap mprotect(), and on Windows it should wrap VirtualProtect().
Assignee: nobody → rohitssingh
Out of curiosity, have we evaluated boost::interprocess's shared memory functionality?
We can't import boost code in general, it utilizes *every* C++ feature and pulls in yet another "base library"; boost would be our third: chromium, XPCOM, and boost.  More specifically boost's use of RTTI and exceptions is a problem, the latter of which pervades the API.

We're using Google Chromium's shared memory implementation for the time being, but it can be improved.  That's what this bug concerns.
> 1.  For bug* *523172: Do you expect us to re-implement the Chromium 
> shared memory implementation, or just encapsulate it to prevent access 
> to GiveToProcess(), Lock(), and Unlock()?  In that case, the only thing 
> we would need to do would add SystemPageSize and Protect.  The 
> encapsulating class can have the concept of a shared memory owner, which 
> is used to allow access to "Protect" the shared memory segment.

I would suggest using the chromium code as a starter, and add/remove as necessary.  It's been working for me so far.  Also check out the work-in-progress patch in bug 523174, which has a dumb "placeholder" implementation of SystemPageSize()/Protect() that you might want to refer to.

We don't need to be able to access shmem by name.  That can be cut out of both the windows and POSIX impls, as it adds a lot of complexity for no gain for us.  (We'll be using fd's instead.)

For POSIX, we should replace their implementation with a simpler one that uses |shm_open()| immediately followed by |shm_unlink()|.  Their current implementation is way more complicated than we need, again for no gain to us.

This would sit on top of low-level shmem code, but we will also need to be smart about re-using allocated shmem segments; we'll eventually need the shmem manager ("smm") you referred to in e-mail correspondence :).  The way that Joe will be using shmem is a lot different than I originally thought, and we'll have more churn of "virtual shmem" allocations than I expected.  It would be nice to push the complexity of managing "real shmem" allocations into library or IPDL code, as you pointed out earlier.

> 2.  Why do we need to forgo thread-safety checks? 

Our graphics folks will be the first shmem consumers, and they'll use shmem to optimize cross-process drawing.  They'll do this by creating a special surface in cairo, a graphics library we use.  Unfortunately, Joe and Jeff tell me that having cairo use a C++ "smart buffer", with thread-safety checks, is too much work.  Instead, we need to create the surface with a raw pointer to the underlying shmem :/.  It would be extremely difficult to do thread-safety checks of raw pointer dereferences, although it /is/ possible.

> 3.  Should I also add a function to export the raw pointer associated 
> with a shared memory segment? 

Yes.  Chromium's implementation is fine.

> 4.  How are processes identified?  Do we have some kind of ProcessHandle 
> we can latch onto? 

Yup, we have ProcessId's and ProcessHandle's, although the distinction is only important on Windows.  We use them in bug 523174 and bug 523175.
I've started coding this up for Unix.  

You're right: A lot of the code can be removed if we avoid passing around file names as a way to reference shared memory segments.  SharedMemoryHandles should be good enough.  

Here are the functions I think we need: 

public:
SharedMemory(size) -> Creates a segment of size.  
SharedMemory(handle) -> Initializes SharedMemory object from its handle.  We may want to put protections to ensure we don't have 2 SharedMemory objects with the same handle.  This is the primary way to share memory across processes.  
~SharedMemory() -> Unmaps shared memory & destroys object.  
static IsHandleValid() -> Checks to ensure a SharedMemoryHandle is valid.  
static NULLHandle() -> Returns -1 (for UNIX).  
ShareToProcess() -> This doesn't reallly share anything -- it just dups the fd.  We should replace this with:
SharedMemoryHandle Duplicate(); -> Dupes the fd/handle so that it can be shared.  
Memory() -> Returns a pointer to the base of the segment.  
MaxSize() -> Returns the segment size.  
Handle() -> Returns the fd/handle.  
Protect() -> Changes access mode.  
static SystemPageSize() -> Returns sysconf(_SC_PAGESIZE).  

private: 
SharedMemory() -> All SharedMemory objects MUST be mapped.  

We can get rid of Create, Delete, Open, Close, Lock, Unlock, GiveToProcess.  

The only tricky detail is ensuring that shared memory files are unique.  There are two ways of accomplishing this:  
* Use shmget with IPC_PRIVATE -> IPC_PRIVATE increments each time we allocate a new page.  This might be bad -- if we get a lot of churning (like you're suggesting we might) -- we might eventually collide with a key some other process wants to use.  This should be fine for prototyping work though.  
* Use shm_open with our own custom filename -> I'm thinking we'd use something like "/ff_electrolysis_$num", where $num is an ever-increasing number.  This is tricky because we want to share $num across all the ff processes.  I think that the SMM can ensure that the page containing $num and the mutex protecting $num is mapped to all pages.  This page should not be exposed through the IPDL.  

Thoughts?
Status: NEW → ASSIGNED
For win32, shared memory is referenced via a globally unique string. See the patch in bug 522299 for an example. Internally once created or opened a shared memory segment it's referenced through a handle value. 

I'll be needing this for windowless drawing, so I'd be happy to implement things for windows.
(In reply to comment #6)
> For win32, shared memory is referenced via a globally unique string. See the
> patch in bug 522299 for an example. Internally once created or opened a shared
> memory segment it's referenced through a handle value. 
> 
> I'll be needing this for windowless drawing, so I'd be happy to implement
> things for windows.

it's -> is
(In reply to comment #5)
> Here are the functions I think we need: 
> 
> public:
> ShareToProcess() -> This doesn't reallly share anything -- it just dups the fd.
>  We should replace this with:
> SharedMemoryHandle Duplicate(); -> Dupes the fd/handle so that it can be
> shared.

Unfortunately, Windows dictates this |ShareToProcess()| interface --- we need to use its DuplicateHandle() syscall, which takes a PID argument naming the process to which to send the HANDLE.

> private: 
> SharedMemory() -> All SharedMemory objects MUST be mapped.  
> 

I very much like this idea.  The only problem is that we should expect that the mapping operation might fail.  And because Mozilla doesn't use C++ exceptions, there's no way to signal the failure from the ctor.  So I'd recommend keeping a |Map(void)| method that returns true/false.

> The only tricky detail is ensuring that shared memory files are unique.  There
> are two ways of accomplishing this: 
 
> * Use shm_open with our own custom filename -> I'm thinking we'd use something
> like "/ff_electrolysis_$num", where $num is an ever-increasing number.  This is
> tricky because we want to share $num across all the ff processes.  I think that
> the SMM can ensure that the page containing $num and the mutex protecting $num
> is mapped to all pages.  This page should not be exposed through the IPDL.  
> 

I prefer that we manage shmem segments entirely through fds/HANDLEs, i.e. create "anonymous" mappings in such a way that it doesn't drain system identifiers.  I think this simplifies resource management.  IPDL-generated code is already the one responsible for tracking these segments, so we don't need to worry about sending around pretty, human-readable IDs.

So on POSIX, I like the approach of 
  do {
    id = "/gecko-%d"% (rand());
    fd = shm_open(id, O_EXCL);
  } while(0 < fd)
  shm_unlink(id);

The equivalent on Windows is CreateFileMapping() with a NULL filename.

> Thoughts?

Nice work!  I also want to make sure you have the "v1" patch in bug 523174, which contains quick implementations of Protect() and SystemPageSize() for Windows.  No need to have to crawl MSDN twice ;).
(In reply to comment #8)
> (In reply to comment #5)
> > Here are the functions I think we need: 
> > 
> > public:
> > ShareToProcess() -> This doesn't reallly share anything -- it just dups the fd.
> >  We should replace this with:
> > SharedMemoryHandle Duplicate(); -> Dupes the fd/handle so that it can be
> > shared.
> 
> Unfortunately, Windows dictates this |ShareToProcess()| interface --- we need
> to use its DuplicateHandle() syscall, which takes a PID argument naming the
> process to which to send the HANDLE.
> 
Good call.  We'll keep that function around.  
> > private: 
> > SharedMemory() -> All SharedMemory objects MUST be mapped.  
> > 
> 
> I very much like this idea.  The only problem is that we should expect that the
> mapping operation might fail.  And because Mozilla doesn't use C++ exceptions,
> there's no way to signal the failure from the ctor.  So I'd recommend keeping a
> |Map(void)| method that returns true/false.
> 
Wow . . . .  No exception support?  I am a little surprised.  

I guess we can have an call similar to what you're proposing called IsMapped().  Anything object that returns false is not a valid object, and can't be re-initialized or anything -- it just needs to be dropped.  
> > The only tricky detail is ensuring that shared memory files are unique.  There
> > are two ways of accomplishing this: 
> 
> > * Use shm_open with our own custom filename -> I'm thinking we'd use something
> > like "/ff_electrolysis_$num", where $num is an ever-increasing number.  This is
> > tricky because we want to share $num across all the ff processes.  I think that
> > the SMM can ensure that the page containing $num and the mutex protecting $num
> > is mapped to all pages.  This page should not be exposed through the IPDL.  
> > 
> 
> I prefer that we manage shmem segments entirely through fds/HANDLEs, i.e.
> create "anonymous" mappings in such a way that it doesn't drain system
> identifiers.  I think this simplifies resource management.  IPDL-generated code
> is already the one responsible for tracking these segments, so we don't need to
> worry about sending around pretty, human-readable IDs.
> 
> So on POSIX, I like the approach of 
>   do {
>     id = "/gecko-%d"% (rand());
>     fd = shm_open(id, O_EXCL);
>   } while(0 < fd)
>   shm_unlink(id);
> 
I did not even know you could do that -- that's a pretty cool trick!  
> The equivalent on Windows is CreateFileMapping() with a NULL filename.
> 
Yeah, I saw that one.  
> > Thoughts?
> 
> Nice work!  I also want to make sure you have the "v1" patch in bug 523174,
> which contains quick implementations of Protect() and SystemPageSize() for
> Windows.  No need to have to crawl MSDN twice ;).
I'll grab a hold of that patch.  

Thanks for the feedback!
(In reply to comment #7)
> (In reply to comment #6)
> > For win32, shared memory is referenced via a globally unique string. See the
> > patch in bug 522299 for an example. Internally once created or opened a shared
> > memory segment it's referenced through a handle value. 
> > 
> > I'll be needing this for windowless drawing, so I'd be happy to implement
> > things for windows.
> 
> it's -> is

Hey Jim, 

Thanks for the feedback.  I think we're just going to take the chromium shmem implementation for now, and rip out pieces we don't need.  

When do you need it for?  

Thanks.
Discussed this with Jeff at the gfx meeting, I'm going to start by using google's shared memory. If shmem turns out to be useful, we can easily convert it over.
Sounds cool.  The interface really shouldn't change too much between the two versions, so you should be fine.
I'm coding up Protect right now -- is it fine the way it is?  It's current signature is:

void Protect(char *aAddr, size_t aSize, int aRights);

Shouldn't this be fine:

bool Protect(Rights aRights);

Instead of crashing, this returns true/false if the mprotect succeeded.  This call applies the protection to the entire shmem region.  

Thoughts?
(In reply to comment #13)
> I'm coding up Protect right now -- is it fine the way it is?  It's current
> signature is:
> 
> void Protect(char *aAddr, size_t aSize, int aRights);
> 
> Shouldn't this be fine:
> 
> bool Protect(Rights aRights);
> 

No.  In DEBUG builds, Shmem allocates sentinel pages in front of and behind the "real" shared segment and protects them with no rights, so as to catch buffer under/overflow.  So it needs to be able to protect subregions of the underlying segment differently.  See the latest patch in bug 523174 for how this is used.

> Instead of crashing, this returns true/false if the mprotect succeeded.  This
> call applies the protection to the entire shmem region.  
> 

The only way this would fail is (1) invalid address/region; (2) bad alignment.  In both cases, we want to crash, those are fatal logic errors.  The only code that ought to be using Protect() is Shmem (hopefully!) so we can guarantee that it won't err.  BTW, the latest patch has an updated Protect() implementation that does a few more checks, might be worth checking out.
Okay, I just checked out the latest patch, and what you are saying makes sense.  

However, couldn't we alter Protect() to only mprotect the valid non-Sentinel shmem subregion for DEBUG builds?  So by default, on construction, the sentinel regions are left unprotected.  Subsequent calls only impact the non-Sentinel regions.  

Are there any other use cases for protecting ranges separately?  

I realize that this may be considered a little "gross" because now both Shmem & SharedMemory must know about the Sentinels, but I just like having a simpler interface.  

Thoughts?
(In reply to comment #15)
> Okay, I just checked out the latest patch, and what you are saying makes sense. 
> 
> However, couldn't we alter Protect() to only mprotect the valid non-Sentinel
> shmem subregion for DEBUG builds?  So by default, on construction, the sentinel
> regions are left unprotected.  Subsequent calls only impact the non-Sentinel
> regions.  
> 

The sentinels are also unprotected to verify the shmem metadata every time a shmem is "passed" over IPC.

> Are there any other use cases for protecting ranges separately?  
> 

Probably not, but I hope that Shmem is 100% of our use cases, so this is "it" ;).

> I realize that this may be considered a little "gross" because now both Shmem &
> SharedMemory must know about the Sentinels, but I just like having a simpler
> interface.  
> 

Unless there's a really really compelling need, clients should be using IPDL Shmem, which has the simplest possible interface: Size() + get().  I'd like to keep SharedMemory a spare abstraction over platform shmem; if we have enough power users who really can't use Shmem, then we can just add a trivial wrapper class on top of SharedMemory that provides a mapping-wide Protect().
Okay, sounds cool.  I'll leave Protect as is in the latest patch.
Okay, I just finished my initial stab at Shmem for Unix.  I ended up stripping out 90% of the chromium code, and leaving in a relatively bare-bones implementation.  I haven't tried compiling this yet -- I've basically uploaded it for review.  

If everything looks cool, I'll cook something similar up for Windows, and then run some tests to ensure it works.  

Comments?
Is there any way we could add an attach method for shared handles? Something that unmaps/closes the existing handle and assigns a new one to the internal mapped_file_?
(In reply to comment #19)
> Is there any way we could add an attach method for shared handles? Something
> that unmaps/closes the existing handle and assigns a new one to the internal
> mapped_file_?

What's the use case for this?  Is it something that can't be implemented using the IPDL interface in bug 523175?  (Sorry about the lag in getting this stuff landed, Joe's been working on gfx blockers :S.)

I would prefer to keep shmem management hidden under the cover of IPDL if at all possible, so that we can transparently implement optimizations like smarter shmem allocators that recycle old segments.
(In reply to comment #20)
> (In reply to comment #19)
> > Is there any way we could add an attach method for shared handles? Something
> > that unmaps/closes the existing handle and assigns a new one to the internal
> > mapped_file_?
> 
> What's the use case for this?  Is it something that can't be implemented using
> the IPDL interface in bug 523175?  (Sorry about the lag in getting this stuff
> landed, Joe's been working on gfx blockers :S.)
> 
> I would prefer to keep shmem management hidden under the cover of IPDL if at
> all possible, so that we can transparently implement optimizations like smarter
> shmem allocators that recycle old segments.

Potentially, I guess this is going to mask the process of handing shmem segments around. My use case looks something like this:

process A: alloc segment S1, map, do something to S1
process A: hands S1 to process B
process B: does something to S1, return
process A: does something with S1

(at this point, both A and B have a cache of S1, it should not be reallocated in the next step.)

process A: hands S1 to process B
(technically, in these cases, A does not need to hand S1 over, it just needs to message B saying, ok, do that again with S1)
process B: does something to S1, return
process A: does something with S1
..

process A: unmap S1, alloc segment S2, map, do something to S2
process A: hands S2 to process B
process B: unmaps S1, does something to S2, return
process A: does something with S2
..

process A: hands S2 to process B
process B: returns
process B: does something to S2
process B: does something to S2
process B: does something to S2
process A: hands S2 to process B
process B: returns, w/previous changes in S2
process A: does something with S2

If IPDL can handle all the nitty gritty in between A and B without re-allocating except when A wants it, then that sounds great.
(In reply to comment #21)
> If IPDL can handle all the nitty gritty in between A and B without
> re-allocating except when A wants it, then that sounds great.

Sure.  If I understand your use case correctly, A "handing off" S1/S2 to B means that A doesn't need to touch S1/S2 until B "returns" it back, right?  If so, you can get away with only using the "simple" IPDL shmem interface (i.e. you probably don't need the extensions from bug 524193).

Also, S1/S2 are conceptually distinct segments, right?  And if A were to call |AllocShmem()| twice for S1 then S2, and was handed back the same underlying segment both times, that would be OK?

We could try to spec out a protocol for this, but I'd need to know a little more about what A and B are, and what "do something" means.
(In reply to comment #22)
> (In reply to comment #21)
> > If IPDL can handle all the nitty gritty in between A and B without
> > re-allocating except when A wants it, then that sounds great.
> 
> Sure.  If I understand your use case correctly, A "handing off" S1/S2 to B
> means that A doesn't need to touch S1/S2 until B "returns" it back, right?

correct. (A is the parent shim, B is the windowless plugin.)

>  If
> so, you can get away with only using the "simple" IPDL shmem interface (i.e.
> you probably don't need the extensions from bug 524193).

Has this already landed someplace? Just curious, I have everything working using google's shared memory right now and am handing the handle over. Would be nice to have use of something built-in.
 
> 
> Also, S1/S2 are conceptually distinct segments, right?  And if A were to call
> |AllocShmem()| twice for S1 then S2, and was handed back the same underlying
> segment both times, that would be OK?

correct. A is discarding S1, in my case due to an increase in dimensions.

On the 'B' side, how would 'B' know S1/S2 were different so that it could drop S1 from it's cache?
(In reply to comment #23)
> Has this already landed someplace? Just curious, I have everything working
> using google's shared memory right now and am handing the handle over. Would be
> nice to have use of something built-in.
> 

No, it's blocked on joedrew r?.  But you can apply the patches in bug 523174 and bug 523175 to play with IPDL shmem.  See ipc/ipdl/test/cxx/PTestShmem.ipdl and ipc/ipdl/test/cxx/TestShmem.cpp for a basic idea of the interface.

> > 
> > Also, S1/S2 are conceptually distinct segments, right?  And if A were to call
> > |AllocShmem()| twice for S1 then S2, and was handed back the same underlying
> > segment both times, that would be OK?
> 
> correct. A is discarding S1, in my case due to an increase in dimensions.
> 
> On the 'B' side, how would 'B' know S1/S2 were different so that it could drop
> S1 from it's cache?

For the time being, you would just create/map a new segment and send it across.  (Right now DeallocShmem() and a recycling allocator are not yet implemented.)  I hadn't thought about a notify-on-dealloc API; you can design it! ;)  What kind of notification interface would you like?
(In reply to comment #8)

> Nice work!  I also want to make sure you have the "v1" patch in bug 523174,
> which contains quick implementations of Protect() and SystemPageSize() for
> Windows.  No need to have to crawl MSDN twice ;).

I tried crawling through the patches in bug 523174, but I couldn't find Protect() for Windows.  Can you paste it into the bug?  

Thanks.
The windows stuff is in ipc/glue/SharedMemory_windows.cpp.  It's a small file, here's the entire contents

#include <windows.h>

#include "mozilla/ipc/SharedMemory.h"

namespace mozilla {
namespace ipc {

void
SharedMemory::SystemProtect(char* aAddr, size_t aSize, int aRights)
{
  DWORD flags;
  if ((aRights & RightsRead) && (aRights & RightsWrite))
    flags = PAGE_READWRITE;
  else if (aRights & RightsRead)
    flags = PAGE_READONLY;
  else
    flags = PAGE_NOACCESS;

  DWORD oldflags;
  if (!VirtualProtect(aAddr, aSize, flags, &oldflags))
    NS_RUNTIMEABORT("can't VirtualProtect()");
}

size_t
SharedMemory::SystemPageSize()
{
  SYSTEM_INFO si;
  GetSystemInfo(&si);
  return si.dwPageSize;
}

} // namespace ipc
} // namespace mozilla
Awesome, thanks!  

I will have a patch ready for review sometime tomorrow.  Once again, I haven't tried compiling any of the stuff, and I expect to find compile errors.  I basically will be uploading the code to get people to review it.  

BTW, I noticed in bug 523174, one of the reviewers mentioned you should avoid including windows.h.  How else am I supposed to access CreateFileMapping, MapViewOfFile, etc?  

Thanks!
(In reply to comment #27)
> BTW, I noticed in bug 523174, one of the reviewers mentioned you should avoid
> including windows.h.  How else am I supposed to access CreateFileMapping,
> MapViewOfFile, etc?  
> 

As in the SharedMemory_windows.cpp code I pasted above, only include it from .cpp files.  The complaint was specifically about windows.h being included in a "public" header.
Here's the patch that has my Windows changes as well as the previously submitted Unix SharedMemory changes.  

I'm going to take Thanksgiving weekend off, but next week I plan on testing out the changes I made to ensure they work.
Attachment #412520 - Attachment is obsolete: true
Improved shared memory patch -- builds, but some issues to be hammered out with Unit Test problems.  This is likely because the new shared memory object is not really cloneable (the older one was).
Attachment #414420 - Attachment is obsolete: true
Blocks: 586353
We're going to be using POSIX shmem on android for the foreseeable future, and we need to not be doing the slow and unnecessary things the chromium code is doing to create shmem on linux.
Blocks: 574512
Cool -- some of my other commitments have cooled off, so I definitely have more time to look at this.  I'll update my working copy and see if I can re-jig the Unit Test to not clone the Shmem object this weekend.
I did not realize just how big a change was made to the base Shmem object.  Chris, you and Joe seem to have created an object-hierarchy for the Shmem object -- why was this done?  Do we want to be able to support different Shmem schemes on the same platform?  

I'm hoping to have the Posix implementation fixed up by the end of this week.  After that, I can start work on the UnitTests and the Windows implementation.
I have been playing around with the earlier patch I submitted.  I realize there were 2 problems with my submission:


- Because of the lack of an lseek, the file backing the shared memory segment was too small, resulting in bus errors on dereference.  
- shm_unlinking the file means that a reader cannot access the pages written by the writer.  

I will fix these two problems in my next submission.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.