BIND 10 #2831: define and implement MemorySegmentMapped
BIND 10 Development
do-not-reply at isc.org
Mon Apr 29 07:53:56 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
-------------------------------------+-------------------------------------
Changes (by muks):
* owner: muks => jinmei
Comment:
Hi Jinmei
Replying to [comment:17 jinmei]:
> > 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.
If it fails on any system, we'll know from the builders. It will not
affect any practical functioning of the code, so this is fine.
> > 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).
About the comments regarding the possibility of a race in `Impl()`,
would creating a `BaseSegment` on a temporary file and then `rename()`
it into place avoid the race?
> > * 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.
Thinking about my original comment, if Boost uses `MAP_SHARED` the test
would always work.
> 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.
Nod.
> > * 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.
* Regarding commit `6eae3579a0dc0ed27f33c3e36cec2ad64be7329d`, I wanted
to see a testcase where the segment is asserted to shrink and the data
in it is tested to be consistent. If we can't do it this way, we
should try to test it some other way.
* Regarding commit `8297b3a7425d2a0de1f1f0f5cb1a4572a1780820`, I thought
the forward order would be a better test as it deletes in FIFO order
(whereas the current code deletes it in LIFO order). I follow your
comment about sizes though.
> > * 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.
It is the only test which fails this way. This Valgrind report is still
reproducible. From the stacktrace, it says `msync()` is writing
uninitialized bytes, which indicates that some pages contents may have
changed and the source of this data may have been uninitialized. Here is
a run with `--track-origins=yes` that tracks the origin of the
uninitialized data:
{{{
[ RUN ] MemorySegmentMappedTest.namedAddress
==19211== Syscall param msync(start) points to uninitialised byte(s)
==19211== at 0x371C00E7F0: __msync_nocancel (syscall-template.S:81)
==19211== by 0x4C2D91F:
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)
==19211== by 0x4C2B294:
isc::util::MemorySegmentMapped::~MemorySegmentMapped()
(memory_segment_mapped.cc:230)
==19211== by 0x4C2B2C8:
isc::util::MemorySegmentMapped::~MemorySegmentMapped()
(memory_segment_mapped.cc:233)
==19211== by 0x4662AE: (anonymous
namespace)::MemorySegmentMappedTest_namedAddress_Test::TestBody()
(checked_delete.hpp:34)
==19211== by 0x52D07F9: void
testing::internal::HandleExceptionsInMethodIfSupported<testing::Test,
void>(testing::Test*, void (testing::Test::*)(), char const*)
(gtest.cc:2090)
==19211== by 0x52C4F68: testing::Test::Run() (gtest.cc:2162)
==19211== by 0x52C5046: testing::TestInfo::Run() (gtest.cc:2338)
==19211== by 0x52C5184: testing::TestCase::Run() (gtest.cc:2445)
==19211== by 0x52C54EC: testing::internal::UnitTestImpl::RunAllTests()
(gtest.cc:4237)
==19211== by 0x52D0379: bool
testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,
bool>(testing::internal::UnitTestImpl*, bool
(testing::internal::UnitTestImpl::*)(), char const*) (gtest.cc:2090)
==19211== by 0x52C47C0: testing::UnitTest::Run() (gtest.cc:3874)
==19211== Address 0x525b0f4 is not stack'd, malloc'd or (recently) free'd
==19211== Uninitialised value was created by a stack allocation
==19211== at 0x4C355BE: void*
boost::interprocess::segment_manager<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>::priv_generic_named_construct<char>(unsigned
char, char const*, unsigned long, bool, bool,
boost::interprocess::ipcdetail::in_place_interface&,
boost::interprocess::iset_index<boost::interprocess::ipcdetail::index_config<char,
boost::interprocess::rbtree_best_fit<boost::interprocess::null_mutex_family,
boost::interprocess::offset_ptr<void, long, unsigned long, 0ul>, 0ul> >
>&, boost::interprocess::ipcdetail::bool_<true>)
(segment_manager.hpp:1071)
==19211==
[ OK ] MemorySegmentMappedTest.namedAddress (118 ms)
}}}
It looks like `find_or_construct()` is causing this.
I have pushed another minor comment update commit to this branch.
Other than the minor task about a testcase for shrink discussed above,
this branch looks good to me now, so you can go ahead and merge it.
--
Ticket URL: <http://bind10.isc.org/ticket/2831#comment:19>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list