BIND 10 #2836: update in-memory zone loading so it can work with shmem segment
BIND 10 Development
do-not-reply at isc.org
Fri May 10 00:34:20 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-20130514
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:12 vorner]:
> Before going to process your comments, I'd like to discuss something.
I'm not
> sure this is the way forward, because catching the SegmentGrown
exception in
> several different places in different layers of abstraction looks
incredibly
> messy and badly designed. I fear this'll end up very buggy and such.
I have to agree with this, and I'm disappointed I was originally too
optimistic about this.
> Where is the layer at which point it should never get through? I believe
we
> need to choose one such layer and not handle it below or above it. Also,
how to
> keep up all the pointers that are cached up to date? Should we keep some
kind
> of offsets instead?
On a closer look, I guess the situation may not be that bad. In
principle, we should still be able to consolidate the point of handling
`MemorySegmentGrown` only at the highest level, which, in the zone
loading case, is `ZoneWriter::load()`:
{{{#!cpp
void
ZoneWriter::load() {
while (!zone_data_) {
try {
zone_data_ = load_action_(segment_.getMemorySegment());
} catch (const MemorySegmentGrown&) {}
}
}
}}}
Actually, this won't work because we can't redo load_action_, but
that's a different issue. And, even if we could restart load_action_,
we wouldn't like to do it because restarting from a scratch can be
very expensive for very large zones. So, we'd at least like to handle
`MemorySegmentGrown` within `ZoneDataUpdater` (in addition to
somewhere outside of it). I guess this point is one possible source
of confusion, making us feel we need to catch this exception
everywhere.
Another tricky point is how to ensure exception safety (in general,
not only specific `MemorySegmentGrown`). The current implementation
of `SegmentObjectHolder` is not enough, because the raw address stored
in the holder may be (indirectly) remapped by the time it needs to be
freed. Once we address this point, however, it seems we can keep
using the current code almost as is. A rough sketch of the revised
`SegmentObjectHolder` would look like this:
{{{#!cpp
template <typename T, typename ARG_T>
class SegmentObjectHolder {
public:
SegmentObjectHolder(util::MemorySegment& mem_sgmt, T* obj, ARG_T arg,
const char* obj_name) :
mem_sgmt_(mem_sgmt), arg_(arg), obj_name_(obj_name)
{
mem_sgmt.setNamedAddress(obj_name_, obj);
}
~SegmentObjectHolder() {
T* obj = getObjectAddress();
if (obj) {
T::destroy(mem_sgmt_, obj, arg_);
mem_sgmt_.clearNamedAddress(obj_name_);
}
}
T* get() { return (getObjectAddress()); }
T* release() {
T* ret = getObjectAddress();
if (ret) {
mem_sgmt.clearNamedAddress(obj_name_);
}
return (ret);
}
private:
T* getObjectAddress() {
std::pair<bool, void*> result =
mem_sgmt_.getNamedAddress(obj_name_);
if (result.first) {
return (static_cast<T*>(result.second));
}
return (NULL);
}
util::MemorySegment& mem_sgmt_;
ARG_T arg_;
const char* const obj_name_;
};
}}}
Then `loadZoneDataInternal()` (in zone_data_loader.cc) would be:
{{{#!cpp
ZoneData*
loadZoneDataInternal(...) {
ZoneData* zone_data = NULL;
while (!zone_data) {
try {
zone_data = ZoneData::create(mem_sgmt, zone_name);
} catch (const MemorySegmentGrown&) {}
}
SegmentObjectHolder<ZoneData, RRClass> holder(mem_sgmt, zone_data,
rrclass,
"tmp_zone_data");
ZoneDataLoader loader(mem_sgmt, rrclass, zone_name, *holder.get());
// the rest of the code is the same
}
}}}
`ZoneData::create` can be almost the same, simply using the updated
`SegmentObjectHolder` (it can propagate `MemorySegmentGrown`).
Same for `ZoneTable::addZone()`; we'd use the revised
`SegmentObjectHolder` and propagate any `MemorySegmentGrown` thrown
from this method. The exception is handled at the higher layer, which
in our case is `ZoneWriter::install()`:
{{{#!cpp
template <typename T>
class NamedAddressHolder {
public:
NamedAddressHolder(MemorySegment& mem_sgmt, T* obj, const char*
obj_name) :
mem_sgmt_(mem_sgmt), obj_name_(obj_name)
{
mem_sgmt_.setNamedAddress(obj_name, obj);
}
~NamedAddressHolder() { mem_sgmt_.clearNamedAddress(obj_name_); }
T* get() {
std::pair<bool, void*> result =
mem_sgmt_.getNamedAddress(obj_name_);
if (result.first) {
return (static_cast<T*>(result.second));
}
return (NULL);
}
private:
MemorySegment& mem_sgmt_;
const char* const obj_name_;
};
void
ZoneWriter::install() {
NamedAddressHolder<ZoneTableHeader>
hdr_addr(segment_.getMemorySegment(),
&segment_.getHeader(),
"ZoneWriter::zt_header");
NamedAddressHolder<ZoneData> zdata_addr(segment_.getMemorySegment(),
zone_dta_,
"ZoneWriter::zone_data");
while (true) {
try {
ZoneTable* table(hdr_addr.get()->getTable());
if (!table) {
isc_throw(isc::Unexpected, "No zone table present");
}
const ZoneTable::AddResult result(table->addZone(
segment_.getMemorySegment(),
rrclass_, origin_,
zdata_addr.get()));
state_ = ZW_INSTALLED;
zone_data_ = result.zone_data;
return;
}
} catch (const MemorySegmentGrown&) {}
}
}}}
I can't say it's simple, but at least I think the design principle is
clarified:
- `MemorySegmentGrown` is generally handled only at the highest layer
- layers lowers than that only have to ensure exception safety (with
the help of `SegmentObjectHolder` if necessary) so the while loop
idiom eventually completes the original goal without loosing any
memory.
- `ZoneDataUpdater` is a lower layer while it catches
`MemorySegmentGrown`,
but it's an exceptional case for performance reasons.
> Or, wouldn't there be a better way than the exception? The segment
growing
> doesn't seem like an exceptional event to me, to tell the truth.
I'm open to alternatives, but I myself haven't come up with an
obviously better idea.
- In general, choices for error (or unusual event) handling are
exceptions and error codes. If we use the latter approach, we'd
examine the result of every call to allocate(), and take some
recovery action if it indicates "memory segment grown". I'm not
sure if it makes the code less messier.
- Another possibility is to try to allocate a sufficiently large
segment at the beginning, and treat any "memory segment grown"
situation as fatal. This would make the implementation much
simpler, but it would be quite difficult to estimate the reasonable
size of initial memory.
> Also, I don't think the zone data updater can keep the named pointer
inside it
> outside of its own functions. What if there are two updaters working on
> different zones in the same segment? Wouldn't they bother each other?
Two updaters on different processes? It's not allowed at least for
`MemorySegmentMapped`. Besides, if two `ZoneDataUpdater` objects work
on the same segment somehow, making a name inside a single method
doesn't help because the name space is global for the segment. If
such a situation can ever happen and we want to worry about it, we
should use different names for different zones. But it's still
independent from where to make the name, either per `ZoneDataUpdater`
instance or within a single method.
--
Ticket URL: <https://bind10.isc.org/ticket/2836#comment:13>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list