BIND 10 #2831: define and implement MemorySegmentMapped

[BIND 10 Development]

#2831: define and implement MemorySegmentMapped
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130423
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  5             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  shared memory data source
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => jinmei


Comment:

 Hi Jinmei

 Replying to [comment:14 jinmei]:
 > Before answering the comments: I realized Boost managed_mapped_file
 > could cause various build time issues: depending on the combination of
 > Boost version, type of compiler (and maybe its version), and the
 > system, it may be `-Werror` unfriendly or in the worst case (Solaris +
 > !SunStudio, as usual) doesn't seem to compile at all.  So far, the
 > only critical failure seemed to be the case for sunstudio, but I had
 > to give up making it compile after trying several things.  Since the
 > shared memory support is a kind of optional feature for limited large
 > scale environments, my suggest at this moment is to introduce a
 > ./configure knob so we can disable building it completely.

 Nod.

 > I updated confiugre.ac and our m4 macro for boost to make it possible,
 > with other portability tricks such as auto-detecting and suppressing
 > `-Werror` regression.
 >
 > These changes are independent from other changes of the branch, so if
 > it's more convenient for you, you can review it separately.  The
 > changes are currently committed at the end of the branch, from e01ee17
 > to ba0d613 (inclusive).

 I have reviewed these as much as I can. I hope you have put this branch
 to the buildbots to check that it builds on different platforms with
 these `configure.ac` changes.

 > > I am trying to follow the design behind why `MemorySegmentGrown` is an
 > > exception, and not a value returned as the method's return value, or
 in
 > > a method argument. It is not an exceptional situation that should be
 > > allowed to unwind the stack all the way upwards, even if by mistake.
 > > `allocate()` can return `NULL` and a `bool` in an argument this case,
 > > which the caller can use and pass on even to its caller.
 >
 > Fair question.  I tried to clarify that in doc.  The specific
 > intended usage is, again, #2836.  I admit the interface and usage
 > is quite tricky, though.  If there's a better way to handle
 > intermediate segment extension in the context of #2836, I'm happy to
 > adopt it instead of the currently implemented approach.

 The example provided looks fine to me.

 > > It also seems inconsistent that `setNamedAddress()` doesn't throw if
 it
 > > allocates memory, which may also cause addresses previously returned
 by
 > > `allocate()` to be relocated (similar to when `allocate()` throws
 > > `MemorySegmentGrown`).
 >
 > Ah, good catch.  I was actually not aware of the inconsistency, but
 > the difference was intentional: `setNamedAddress()` is generally
 > expected to be independently, not as part of a set of allocations.
 > I clarified it in the doc (10d88f0).

 Ok.

 > > * The `setNamedAddress()` API doc does not describe what happens if
 > >   `NULL` is passed in the `addr` argument. If `NULL` will clear the
 > >   association, then `clearNamedAddress()` would be redundant (we
 should
 > >   not have to support two different ways of clearing the
 > >   association). If indeed `NULL` can be associated with a name, it
 > >   should be documented (with what a sample use-case may be for storing
 > >   `NULL`).
 >
 > Hmm, I thought it was at least clear NULL is allowed, but I tried to
 > explain more details about what it would mean.

 That is what I had meant too. The previous doc stated that it accepted
 NULL, but whether it cleared any association or just stored NULL was
 unclear. It's clear now.

 > > * In the API doc of `MemorySegmentMapped()`, is it assumed that if
 > >   `create` argument is true, then any existing file will be
 overwritten
 > >   (truncated)?  If this is not the case, can the `create` argument be
 > >   removed, such that this read+write version of
 `MemorySegmentMapped()`
 > >   auto-creates the file if it doesn't exist, or opens it if it does
 > >   exist?
 >
 > Not a direct answer to these questions, but I realized we'd also need
 > a "create only" mode for our later work, so I revised the interface
 > quite substantially.  Please check the new `OpenMode` enum, revised
 > constructor, and their documentation.

 Nod.

 > > * Is there a reason why you use 0 instead of `NULL` in a lot of
 places,
 > >   including unittests? (They are equivalent, but `NULL` is a gentle
 > >   reminder that it's a pointer).
 > > {{{
 > > void*
 > > MemorySegmentLocal::getNamedAddress(const char* name) {
 > >     std::map<std::string, void*>::iterator found =
 named_addrs_.find(name);
 > >     if (found != named_addrs_.end()) {
 > >         return (found->second);
 > >     }
 > >     return (0);
 > > }
 > > }}}
 >
 > Please see http://bind10.isc.org/ticket/2833#comment:16 (search for
 > "NULL") and the followup discussion.  I've reverted to NULL for this
 > branch, too.

 Nod. I have pushed a commit fixing a couple of other cases too.

 > > * I think this public variable's definition should go into a `.cc`
 file:
 > > {{{
 > >     /// \brief The default value of the mapped file size when newly
 created.
 > >     ///
 > >     /// Its value, 32KB, is an arbitrary choice, but considered to be
 > >     /// sufficiently but not too large.
 > >     static const size_t INITIAL_SIZE = 32768;
 > > }}}
 >
 > Good catch, done.

 I don't know if it's over the top to add a check like this:
 {{{
 TEST_F(MemorySegmentMappedTest, staticVariables) {
     // Attempt to take address of MemorySegmentMapped::INITIAL_SIZE.
     EXPECT_EQ(DEFAULT_INITIAL_SIZE,
 *(&MemorySegmentMapped::INITIAL_SIZE));
 }
 }}}

 > > * Can the following comment be removed from
 > > `MemorySegmentMapped::setNamedAddress()`? It seems weird reading it by
 > > itself (it is as if by default the base `MemorySegment` interface
 > > doesn't expect `true` to be returned):
 > > {{{
 > >     /// This version can actually return true.
 > >     ///
 > > }}}
 >
 > I can do it, but I thought it's useful to add note specific
 > differences of specific implementations (of course the caller
 > shouldn't assume the derived-class specific behavior when it uses the
 > abstract interface).  I updated the documentation in that sense
 > at b8d905c.  But if you think it's still rather confusing, I'm okay
 > with removing it.

 When reading it, explicitly pointing out that it could actually return
 `true` came across to be somewhat weird, as the interface requires the
 code to expect both. It was as if it implied that normally `false` is
 returned by the interface.

 Instead of this line, a line that just states when true will be returned
 and when false will be returned will be more readable there.

 > > * `getNamedAddress()` has different behavior in the different
 > > implementations with `name=NULL`.  If we don't want to support
 > > `name=NULL` , it should be documented in the
 > > `MemorySegment::getNamedAddress()` base class interface.
 > [...]
 > > * Same in `clearNamedAddress()` too.
 >
 > Okay.  In general, I'd prefer explicitly rejecting invalid (or
 > unexpected) parameters, so I revised the implementation and
 > documentation that way.

 Nod. This looks fine.

 > > * What are the conditions under which this assertion is triggered?
 i.e.,
 > >   how did you arrive at this `INITIAL_SIZE` in the condition
 > >   expression?
 > > {{{
 > >     // It appears an assertion failure is triggered within Boost if
 the size
 > >     // is too small.  To work this around we'll make it no-op if the
 size is
 > >     // already reasonably small.
 > >     if (getSize() < INITIAL_SIZE) {
 > >         return;
 > >     }
 > > }}}
 >
 > I found it through our unit tests (if I disabled the above 'if', one
 > of the unit tests triggered the assertion failure).  I tried to
 > clarify the issue in more detail as a code comment.

 Ok.

 > > * Can something like `MappedFile` be used as the type name instead of
 > >   `BaseSegment`? The method calls on `base_sgmt_` seem to match a file
 > >   naming.
 >
 > I don't understand the second sentence...anyway, I'm okay with
 > renaming it, but boost::basic_managed_mapped_file actually comes from
 > the "Managed Memory Segments" abstraction
 > (see
 >
 http://www.boost.org/doc/libs/1_53_0/doc/html/interprocess/managed_memory_segments.html),
 > rather than file abstraction, so I prefer using the term "memory"
 > and/or "segment".
 >
 > If `BaseSegment` sounds confusing to you, I propose, e.g.,
 > `BoostSegment` and `boost_sgmt_`.  At the moment I didn't change the
 > code.
 >

 Ok. This is a good enough explanation.

 > > * A general observation about API doc comments, which applies to the
 > >   rest of BIND 10 API doc too: differently written comments seem to be
 > >   used to say the same thing about exceptions:
 > >
 > > {{{
 > >     /// This version never throws.
 > > }}}
 > >
 > > and
 > >
 > > {{{
 > >     /// \throw None
 > > }}}
 > >
 > > Documentation about exceptions that can be thrown:
 > [...]
 > > Shall we use `\throw` in all cases to be consistent? It would be
 easier
 > > when we get around to publishing such API documentation for the BIND
 10
 > > libraries.
 >
 > As a general matter I tend to agree (at least it's better to be
 > consistent, and use of `\throw` would be the best choice), but
 > regarding this one:
 > {{{
 >     /// This version never throws.
 > }}}
 > it's rather implementation note than part of API spec (Basically, as
 > API the base class description should be referenced).
 >
 > For this branch, I've added some more `\throw`s where they seem
 > appropriate.  I also changed some of `InvalidOperation` cases to
 > `MemorySegmentError` as it should be basically generic enough at the
 > base class level.

 These look fine.

 > > * This is not very important, but I wanted to suggest using XOR
 instead
 > >   of addition in the checksum computation. I checked about this to
 make
 > >   sure before suggesting it, and it seems that for 2-bit errors,
 > >   addition has a slightly better probability of generating a different
 > >   checksum. So addition is good in this case.
 >
 > This checksum is basically quite poor anyway in terms of integrity
 > check, so the difference in that sense is subtle anyway.  Nevertheless
 > I'm okay to switch to any other algorithm as long as it's lightweight,
 > but as it seems there's no reason to change it for now, I've kept it.

 Nod.

 > > * A test should be added to verify that previous name/address
 > >   associations continue to exist and are still usable after
 > >   `shrinkToFit()` is called, and previously allocated data exists and
 is
 > >   the same. For these names and data, I suggest using random sizes and
 > >   random data, a list of which you can save in `vector`s for matching
 > >   after the shrinking is performed.
 >
 > Okay, added these cases to the `namedAddress` test.

 This looks fine. I have pushed commit(s) modifying this code.

 > > * For the above case, we should also test the case where the mapping
 > >   changes (somehow force it to happen). In this case, we test that
 > >   `getNamedAddress()` returns the updated address and the data at that
 > >   address is consistent with what we expect.
 >
 > Also added it to the `namedAddress` test.

 This also looks fine.

 > > * Is it not possible to perform `shrink_to_fit()` when the
 `BaseSegment`
 > >   object is still alive (and the mapped file is open)? The
 documentation
 > >   doesn't seem to say anything for or against it. Same question
 applies
 > >   to `grow()` used in `growSegment()` too.
 >
 > Do you mean whether the underlying Boost library allows that (and
 > whether we really need reset `BaseSegment` before grow/shrink)?  If
 > so, I thought Boost doesn't allow it, but, hmm, according to
 >
 http://www.boost.org/doc/libs/1_53_0/doc/html/boost/interprocess/basic_managed_mapped_file.html
 > it may not so be clear.  It could depend on what reading/writing
 > means.  But from an additional quick experiment, if we keep
 > `BaseSegment` and perform grow(), a subsequently call to allocate()
 > caused crash, so I guess we cannot really assume it works that way.
 > I suspect that's the same for shrink.
 >
 > ...but are we on the same page, or are you asking something different?

 In this case, what the current code does is correct and required, so
 it's fine.

 > > * I don't see any fork tests for interprocess testing. Please add
 these,
 > >   as all of `MemorySegmentMapped`'s methods and the allocated data
 ought
 > >   to be tested to function properly from independent process address
 > >   spaces with a shared map file. This is our use-case.
 >
 > Okay, added.

 This looks fine. I have pushed commit(s) modifying this code.

 If it's not too much, I'd like to see multiple reader processes (you
 fork() a few more times). To synchronize, you could make them map first,
 then sleep for a few seconds so that they all have had time to map, and
 then read and write the byte to their corresponding pipe. But it's up to
 you, if you think such testing is necessary.

 > > * We should also have tests for write behavior.. i.e., are writes to
 > >   memory reflected in the maps from other reader processes before the
 > >   writer destroys its segment? Are they when the writer destroys its
 > >   segment?
 >
 > These don't work, or more accurately, we cannot guarantee that.  Our
 > readers open the mapped memory in read-only mode, which internally
 > means they use it as a private memory space.  In this case it's not
 > even guaranteed that modifications to the original file is ever
 > reflected to reader's memory (different OSes showed different
 > behaviors in my previous experiments).  We basically don't allow
 > mixture of writer and readers (and that's the responsibility of the
 > implementor and operator).

 Ok.. if readers and writers don't mix, then that's fine.

 > > * I don't understand this assertion in `growSegment()`:
 > > {{{
 > >         const size_t new_size = prev_size * 2;
 > >     assert(new_size != 0); // assume grow fails before size overflow
 > > }}}
 >
 > I've revised a comment to clarify the intent at commit da33bc4.  Does
 > that answer your question?

 I meant to ask about how the code checks for overflows using that
 assertion. The current code will work for sizes which are powers of 2
 where, when only the MSB is set, multiplying size by 2 will result in
 0. But we make no such assertion in the constructor, nor document that
 only powers of 2 are accepted as sizes.

 A better assertion would be:
 {{{
          // behavior.  But we basically assume grow() would fail before
 this
          // happens, so we assert it shouldn't happen.
          const size_t new_size = prev_size * 2;
 -        assert(new_size != 0);
 +        assert(new_size > prev_size);
 }}}

 > > * Is this guaranteed by Boost?
 > > {{{
 > >     // Another shrink shouldn't cause disruption, and the size
 shouldn't change
 > >     segment_->shrinkToFit();
 > >     EXPECT_EQ(shrinked_size, segment_->getSize());
 > > }}}
 >
 > Boost doesn't guarantee it, but our wrapper (basically) does.  See the
 > discussion of "perform `shrink_to_fit()` with `BaseSegment` still
 > alive" above.

 The discussion that you refer to says that it's required to close the
 mapped file before shrinking/growing. Here, my question was, is it
 guaranteed that a second shrink will not change the size?

 > > * If `initial_size` is passed as 0 to the constructor, Boost (or
 > >   something else under it) throws a library error. Also 0 passed to
 > >   `allocate()` returns a non-NULL pointer. Not sure if we should
 bother
 > >   to add lines to the API doc about these extreme cases, whether they
 > >   are allowed or not, or assertions.
 >
 > As for the initial_size, Boost rejects it if it's too small because it
 > internally needs to allocate some management data structures.  But its
 > size is not public information (or even the existence of such
 > restriction is not publicly described), so we cannot predict the
 > exact threshold.  it should be safe to assume 0 will be always
 > rejected, so I added a test case for it and added notes about it in
 > the documentation.

 Nod.

 > Regarding allocate() and deallocate(), I think it's okay to say
 > nothing.  After all, the user shouldn't use the return value anyway,
 > and as long as deallocate() works for any pointer value returned by
 > allocate(), it's not really special.

 This is reasonable.

 > > * Though it is an advisory lock, it may make sense to additionally
 make
 > >   use of `fcntl()` read and write locks to check for mutual exclusion
 in
 > >   `MemorySegmentMapped`. This will make visible any programming
 mistakes
 > >   quickly.
 >
 > Do you mean acquiring a lock when opening a segment in read-write
 > mode (so there should be no more than one writer at the same time)?

 Yes, and also acquiring read locks so that that there are NO writers
 when there are readers.

 I feel this should be implemented inside `MemorySegmentMapped` itself to
 avoid programming issues in higher layers.

 * Is the code following this comment guaranteed to work?
 {{{
     // Open a separate read-only segment and checks the same named data
     // Since the mapped space should be different, the resulting bare
 address
     // from getNamedAddress should also be different, but it should
 actually
     // point to the same data.
     // Note: it's mostly violation of the API assumption to open read-only
     // and read-write segments at the same time, but unless we modify the
     // segment throughout the lifetime of the read-only segment, it should
     // work.
 }}}

 We have not flushed our writer segment at this point. Will setting up
 another map to the same address space show up the modified memory every
 time we run this code? `make check` passed for me so far, but I wonder
 if this is guaranteed.

 * Aside from minor comment updates, I have pushed some code fixes to the
   branch. Please check these.

 * This branch generates a Valgrind failure:
 {{{
 [ RUN      ] MemorySegmentMappedTest.namedAddress
 ==17022== Syscall param msync(start) points to uninitialised byte(s)
 ==17022==    at 0x371C00E7F0: __msync_nocancel (syscall-template.S:81)
 ==17022==    by 0x4C2CB3F:
 boost::interprocess::basic_managed_mapped_file<char,
 boost::interprocess::rbtree_best_fit<boost::interprocess::null_mutex_family,
 boost::interprocess::offset_ptr<void, long, unsigned long, 0ul>, 0ul>,
 boost::interprocess::iset_index>::flush() (mapped_region.hpp:540)
 ==17022==    by 0x4C2AA44:
 isc::util::MemorySegmentMapped::~MemorySegmentMapped()
 (memory_segment_mapped.cc:152)
 ==17022==    by 0x4C2AA78:
 isc::util::MemorySegmentMapped::~MemorySegmentMapped()
 (memory_segment_mapped.cc:155)
 ==17022==    by 0x466FE7: (anonymous
 namespace)::MemorySegmentMappedTest_namedAddress_Test::TestBody()
 (checked_delete.hpp:34)
 ==17022==    by 0x52CF7F9: void
 testing::internal::HandleExceptionsInMethodIfSupported<testing::Test,
 void>(testing::Test*, void (testing::Test::*)(), char const*)
 (gtest.cc:2090)
 ==17022==    by 0x52C3F68: testing::Test::Run() (gtest.cc:2162)
 ==17022==    by 0x52C4046: testing::TestInfo::Run() (gtest.cc:2338)
 ==17022==    by 0x52C4184: testing::TestCase::Run() (gtest.cc:2445)
 ==17022==    by 0x52C44EC: testing::internal::UnitTestImpl::RunAllTests()
 (gtest.cc:4237)
 ==17022==    by 0x52CF379: bool
 testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,
 bool>(testing::internal::UnitTestImpl*, bool
 (testing::internal::UnitTestImpl::*)(), char const*) (gtest.cc:2090)
 ==17022==    by 0x52C37C0: testing::UnitTest::Run() (gtest.cc:3874)
 ==17022==  Address 0x525a0f4 is not stack'd, malloc'd or (recently) free'd
 ==17022==
 [       OK ] MemorySegmentMappedTest.namedAddress (84 ms)
 }}}

-- 
Ticket URL: <http://bind10.isc.org/ticket/2831#comment:16>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development