BIND 10 #2836: update in-memory zone loading so it can work with shmem segment
BIND 10 Development
do-not-reply at isc.org
Tue May 14 19:09:38 UTC 2013
#2836: update in-memory zone loading so it can work with shmem segment
-------------------------------------+-------------------------------------
Reporter: jinmei | Owner:
Type: task | jinmei
Priority: medium | Status:
Component: data source | reviewing
Keywords: | Milestone:
Sensitive: 0 | Sprint-20130528
Sub-Project: DNS | Resolution:
Estimated Difficulty: 4 | 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:17 vorner]:
> > - whether it's in add() or the constructor, there's a very slight
> > chance that `setNamedAddress()` itself causes address remapping.
> > In #2850 we are discussing making it safer/more convenient.
> > You may want to use the result (and in any case you may have to
> > update this branch if it causes interface change), or otherwise
> > you'll have to call `getNamedAddress()` immediately after
> > `setNamedAddress()` to make sure the address is really the latest
> > one.
>
> OK, once the branch is in master, I'll go through it and see what needs
to be updated. I'd like to avoid too many merges now.
Part of #2850 has been merged to master. I believe all that this
branch needs are now there, and won't require future changes due to
the rest of #2850 discussions. That is, you should now be able to
merge/incorporate it for this branch safely.
> > - `ZoneDataUpdaterTest`: is there a reason `mem_sgmt_` is a shared
> > pointer, not scoped?
>
> The reason is, I pass it as a return value from the creator and copy it.
Wouldn't a scoped pointer destroy it once the returned instance was
destroyed?
Ah, okay. Actually scoped pointer is even non copyable. So it can't
be used as a return value of a function. In this test usage, however,
`SegmentCreator::create()` seems to be able to just return the result
of `new` and have the caller store it in a scoped_ptr. But that's not
a big deal and mostly a matter of taste.
> > - `MappedSegmentCreator::cleanup`:
`boost::interprocess::file_mapping::remove`
> > is probably better than unlink. It would be more portable (at least
> > to the extent of the theory that Boost is expected to be portable),
> > and it also seems to have some consideration on mmap specifics (and
> > so safer).
>
> I can't seem to find the function in the documentation. But I found
this:
>
> {{{
> To remove the file from the filesystem you can use standard C
std::remove or Boost.Filesystem's remove() functions.
> }}}
>
> So, unlink seems to be the same kind of function as they propose.
Hmm, it also says `file_mapping::remove` is "more portable", but I
have to say I really don't understand the difference between these.
So I'd leave it to you.
> I also pushed the test that crashes for me. Does it crash for you too?
Does valgrind work for you? If so, could you run it through it?
That test crashed in my environment, too (though in a different place
of the code). I've found one issue and committed a fix to that (my)
problem at 575854a507052ad7de51aa259fd5d8f04156d15d on branch
trac2836-2. Hopefully it will fix yours, too. Note that we may or
may not use the 5758... fix as it is (see below).
Specific comments on the code:
'''domaintree.h'''
- The change itself looks okay, but I noticed the previous paragraph
is not entirely correct:
{{{#!cpp
/// method is called. For example, the result of \c find() should be
/// the same. This method provides the weak exception guarantee in
its
}}}
An incomplete insert() (due to exception in the middle of it) could
cause a new empty node as a result of `nodeFission()`, which will
now be found by find() if the domain tree was constructed
accordingly. I suggest combining and updating several related
paragraphs as follows:
{{{#!cpp
/// doesn't exist.
///
/// This method normally involves resource allocation. If it fails
/// \c std::bad_alloc will be thrown. Also, depending on details the
/// specific \c MemorySegment, it can propagate the \c
MemorySegmentGrown
/// exception.
///
/// This method does not provide the strong exception guarantee in its
/// strict sense; there can be new empty nodes that are superdomains
of
/// the domain to be inserted as a side effect. However, the tree
/// retains internal integrity otherwise, and, in particular, the
intended
/// insert operation is "resumable": if the \c insert() method is
called
/// again with the same argument after resolving the cause of the
/// exception (possibly multiple times), it should now succeed. Note,
/// however, that i case of \c MemorySegmentGrown the address of the
/// `DomainTree` object may have been reallocated if it was created
with
/// the same \c MemorySegment (which will often be the case in
practice).
/// So the caller may have to re-get the address before calling \c
insert
/// again. It can be done using the concept of "named addresses" of
/// \c MemorySegment, or the direct caller may not have to worry about
it
/// if this condition is guaranteed at a higher level.
///
/// \param mem_sgmt A \c MemorySegment object for allocating memory of
}}}
'''segment_object_holder.cc'''
- maybe we should assert the index never overflows (in practice):
{{{#!cpp
++index;
assert(index != 0); // in practice we should be able to assume this
return ("Segment object holder auto name " +
boost::lexical_cast<std::string>(index));
}}}
(we'll lose one index (=0) this way, though)
- and, instead of size_t, uint64_t may be safer; as far as I know
size_t is often 32-bit for 32-bit machines
'''segment_object_holder.h'''
- maybe we'd note getNextHolderName() is not thread safe (but should
be okay as a memory segment isn't expected to be modified by multiple
threads at the same time in the first place)
- as commented already, we probably want to delay setting `obj` in a
separate method than the constructor.
- also, due to the issue I addressed in
4774e482497ea5ea4326a9c3fe340d5cae56dd8e (of trac2836-2), it's
probably safer if we throw if `setNamedAddress` caused possible
reallocation:
{{{#!cpp
SegmentObjectHolder(util::MemorySegment& mem_sgmt, ARG_T arg) :
mem_sgmt_(mem_sgmt), arg_(arg),
holder_name_(getNextHolderName()), holding_(true)
{
if (mem_sgmt_.setNamedAddress(holder_name_.c_str(), NULL)) {
mem_sgmt_.clearNamedAddress(holder_name_.c_str());
isc_throw(util::MemorySegmentGrown,
"need restart for constructing
SegmentObjectHolder");
}
}
void set(T* obj) {
// since we reserve the space for this named address in the memory
// segment, this shouldn't cause reallocation
const bool grown = mem_sgmt_.setNamedAddress(holder_name_.c_str(),
obj);
assert(!grown);
}
}}}
This way, we shouldn't need 4774e48 and be able to use the
"top-level loop and retry" idiom.
'''zone_writer_local.cc'''
- `ZoneWriterLocal::install`: with the merged part of #2850, you'll
need to call `resetHeader()` in handling `MemorySegmentGrown`.
- also note that in #2850 (part of it merged in master) this file is
renamed and we now have a single (non abstract) `ZoneWriter` class.
- depending on how we revise `SegmentObjectHolder`, `result.zone_data`
could also be relocated on return of `addZone`.
'''segment_object_holder_unittest.cc'''
- (my mistake) the revised 'grow' test could even cause build failure if
`!USE_SHARED_MEMORY` due to the use of managed_mapped_file. So it
should be:
{{{#!cpp
#ifdef USE_SHARED_MEMORY
TEST(SegmentObjectHolderTest, grow) {
... (entire test)
EXPECT_EQ(0, unlink(mapped_file));
}
#endif
}}}
'''zone_finder_unittest.cc'''
- what's the purpose of changing `updater_` to (scoped) pointer?
--
Ticket URL: <http://bind10.isc.org/ticket/2836#comment:18>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list