BIND 10 #2831: define and implement MemorySegmentMapped
BIND 10 Development
do-not-reply at isc.org
Sat Apr 13 07:01:42 UTC 2013
#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
-------------------------------------+-------------------------------------
Comment (by jinmei):
Replying to [comment:16 muks]:
> > 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.
Yes, I've run it. I've not seen an error except one that we've
occasionally seen in the master (so I believe it's not due to this
branch).
> > > * I think this public variable's definition should go into a `.cc`
file:
[...]
> > 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));
> }
> }}}
Might be over the top, but I see it could be useful. So I added it.
> > > * Can the following comment be removed from
[...]
> > 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.
Okay, revised it that way.
> > > * 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
[...]
> Ok. This is a good enough explanation.
To make it clear, I've not changed the name.
> > > * I don't understand this assertion in `growSegment()`:
[...]
> 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);
> }}}
Ah, okay, you were right. I've adjusted the code.
> > > * 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?
Oh, I see, and I'm afraid we cannot guarantee that. I've kept the
test as it actually seems to work in practice but revised the comment.
> > > * 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.
Okay, I've tried to implement it. It resulted in non trivial changes
to the documentation (it now more clearly says reader and writer or
multi writers can't be mixed).
> * 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.
You're probably right, and the "violation" is now almost prohibited
(file lock may not actually detect it on the same process but it's
undefined so we cannot rely on it). I don't think we need to
reproduce the situation of having two different base addresses in the
scope of this test, so I simply remove this block of test.
> * Aside from minor comment updates, I have pushed some code fixes to the
> branch. Please check these.
These are generally look okay, but I reverted or further updated some
of them:
8297b3a [2831] reverted the change for deleting checkNamedData().
6eae357 [2831] removed test for segment size after shrink.
> * 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)
> [ OK ] MemorySegmentMappedTest.namedAddress (84 ms)
> }}}
Hmm, I have no idea if this is the only that fails this way, but it
may be related to the fact that we kept a read-only segment then
removed the file and re-opened it from the scratch. The latest
version makes sure the read-only segment is destroyed first, so it may
change the result.
--
Ticket URL: <http://bind10.isc.org/ticket/2831#comment:17>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list