The attached patch changes the allocation semantics of the cached Xrl
pointer to be per-client-instance rather than per-library-instance.
It does this by modifying the Xrl client stub generator to include a
named auto_ptr<Xrl> for each method in the Xrl client stub.
Sizes are for FreeBSD 7.2/amd64 with gcc 4.2.1. Hopefully it should give
others a feel for working with the code generator.
This probably doesn't fix the original race, but it may make it
easier to mitigate some other way, by giving us a handle on the cached
Xrls which are causing the problem, rather than letting them dangle in
the BSS segment. That, and it should fix the glaringly obvious memory
leakage, and non-reentrancy, caused by polluting a library with 'static
Xrl*' instances.
I've tested this briefly with the current SVN tree, just by running
a xorp_finder, call_xrl, and a xorp_rib. This exercises the reentrancy
of the XRL client stubs, because the Finder Client is instantiated once
for every XrlRouter. The RIB uses libfeaclient, which instantiates its
*own* XrlRouter instance (yes, you can have more than one per
process...) for getting interface updates from the FEA.
This patch does increase the code size of the stubs very slightly;
that's the trade-off. As we are no longer holding the cached 'static
Xrl*' in the BSS segment, it shrinks versus the code segment;
auto_ptr<T> doesn't use any additional data storage above the pointer
type, but we are now checking allocations and deallocations of Xrl for
the stubs.
When building with shared libraries, the template expansions for
auto_ptr are a lost opportunity for coalescing with the linker, although
this only wastes ~128 bytes per library (for auto_ptr<Xrl>::get() and
auto_ptr<Xrl>::reset()). Most of this seems to be NOP padding for cache
line alignment.
I'm still not 100% happy with how the XrlPFSender cache mechanism
works. Because we're holding a pointer somewhere, to something whose
lifecycle is managed somewhere else, there really isn't any other answer
than the one you've already suggested.
I'm not that happy about using ref_ptr<T> to do it, for much the
same reasons as I've already described earlier in this thread -- there
is no clean syntax for observing a ref_ptr vs Boost's shared_ptr/weak_ptr.
I'm not at all happy about using ref_ptr<T>&, because it is all too
easy to ignore its specific meaning and introduce problems. Certainly,
the only reason I did it in Spt, was because that class was already
using ref_ptr<T> internally to track nodes in a container.
Granted, this is code fairly deep down in the core of the tree,
which many developers would never need to touch directly, but that makes
it even more important to be careful.
In Thrift, the binary blobs themselves can be decoupled from where
they go. There is a potential chicken-and-egg problem if we support
multiple TProtocol types, where we would need to know the sender before
the stubs create the blob; if we just speak TBinaryProtocol to
everything, we don't have this problem, but it does mean we can't just
tell the XORP RPC endpoints 'speak JSON to this guy', 'speak XML-RPC to
this guy', 'speak AMQP to this guy' etc.
So the idea of caching the transport we'd prefer to transmit from,
is still one that bears further scrutiny, even in a re-spin. The
difference is, for maximum flexibility, Thrifted XRL stubs would
actually want to see XrlPFSender's equivalent upfront, before
XrlSender::send() is even called.
This is after scrutinizing libxipc even further this week, and
realizing most of what's there is to support asynchrony.
text data bss dec hex filename
156243 4560 56 160859 2745b libbgpxif.so
31757 1456 8 33221 81c5 libclimanagerxif.so
18387 1456 56 19899 4dbb libcliprocessorxif.so
26198 1680 56 27934 6d1e libcommonxif.so
26123 1760 56 27939 6d23 libcoordxif.so
19243 1376 8 20627 5093 libdatainxif.so
38481 1504 8 39993 9c39 libfeaclickxif.so
29171 1512 8 30691 77e3 libfeafibclientxif.so
18825 1320 8 20153 4eb9 libfeafibxif.so
61204 2400 56 63660 f8ac libfeafirewallxif.so
71143 1768 8 72919 11cd7 libfeaifmgrmirrorxif.so
15623 1272 8 16903 4207 libfeaifmgrreplicatorxif.so
150832 4136 56 155024 25d90 libfeaifmgrxif.so
16794 1360 8 18162 46f2 libfearawlinkclientxif.so
23561 1424 8 24993 61a1 libfearawlinkxif.so
18517 1424 8 19949 4ded libfearawpkt4clientxif.so
24473 1456 8 25937 6551 libfearawpkt4xif.so
19420 1456 8 20884 5194 libfearawpkt6clientxif.so
25376 1488 8 26872 68f8 libfearawpkt6xif.so
17594 1288 8 18890 49ca libfib2mribxif.so
24001 1608 56 25665 6441 libfinderclientxif.so
18569 1288 8 19865 4d99 libfindereventnotifierxif.so
15687 1272 8 16967 4247 libfindereventobserverxif.so
60566 2624 56 63246 f70e libfinderxif.so
66026 2656 56 68738 10c82 libftixif.so
24535 1448 8 25991 6587 libmfeaclientxif.so
60890 1928 56 62874 f59a libmfeaxif.so
21082 1384 8 22474 57ca libmld6igmpclientxif.so
72582 2216 56 74854 12466 libmld6igmpxif.so
107675 3400 56 111131 1b21b libospfv2xif.so
109056 3440 56 112552 1b7a8 libospfv3xif.so
387552 5144 56 392752 5fe30 libpimxif.so
17276 1312 8 18596 48a4 libpolicybackendxif.so
18938 1400 8 20346 4f7a libpolicyredist4xif.so
18938 1400 8 20346 4f7a libpolicyredist6xif.so
39396 1712 56 41164 a0cc libpolicyxif.so
16329 1304 8 17641 44e9 libprofileclientxif.so
24369 1600 56 26025 65a9 libprofilexif.so
20864 1384 8 22256 56f0 libredist4xif.so
20864 1384 8 22256 56f0 libredist6xif.so
28993 1680 56 30729 7809 libredisttransaction4xif.so
28993 1680 56 30729 7809 libredisttransaction6xif.so
20856 1384 8 22248 56e8 libribclientxif.so
129643 3752 56 133451 2094b libribxif.so
122275 3704 56 126035 1ec53 libripngxif.so
127945 3736 56 131737 20299 libripxif.so
20872 1360 8 22240 56e0 librtrmgrclientxif.so
71355 3008 56 74419 122b3 librtrmgrxif.so
28119 1704 56 29879 74b7 libsocket4userxif.so
49645 1832 56 51533 c94d libsocket4xif.so
28119 1704 56 29879 74b7 libsocket6userxif.so
45034 1808 56 46898 b732 libsocket6xif.so
59187 1672 8 60867 edc3 libstaticroutesxif.so
26859 1432 8 28299 6e8b libtestpeerxif.so
29075 1808 56 30939 78db libtestxif.so
28140 1600 8 29748 7434 libtestxrlsxif.so
56229 2528 56 58813 e5bd libvrrpxif.so
text data bss dec hex filename
153761 4536 456 158753 26c21 libbgpxif.so
30503 1432 120 32055 7d37 libclimanagerxif.so
17860 1432 64 19356 4b9c libcliprocessorxif.so
25560 1656 80 27296 6aa0 libcommonxif.so
25453 1736 80 27269 6a85 libcoordxif.so
18620 1352 40 20012 4e2c libdatainxif.so
36791 1480 168 38439 9627 libfeaclickxif.so
28467 1488 80 30035 7553 libfeafibclientxif.so
18117 1296 48 19461 4c05 libfeafibxif.so
60227 2376 176 62779 f53b libfeafirewallxif.so
68732 1744 336 70812 1149c libfeaifmgrmirrorxif.so
14981 1248 32 16261 3f85 libfeaifmgrreplicatorxif.so
147502 4112 512 152126 2523e libfeaifmgrxif.so
16267 1336 24 17627 44db libfearawlinkclientxif.so
22962 1400 56 24418 5f62 libfearawlinkxif.so
18022 1400 24 19446 4bf6 libfearawpkt4clientxif.so
23770 1432 56 25258 62aa libfearawpkt4xif.so
18925 1432 24 20381 4f9d libfearawpkt6clientxif.so
24809 1464 56 26329 66d9 libfearawpkt6xif.so
16886 1264 48 18198 4716 libfib2mribxif.so
23260 1584 88 24932 6164 libfinderclientxif.so
17799 1264 48 19111 4aa7 libfindereventnotifierxif.so
15047 1248 32 16327 3fc7 libfindereventobserverxif.so
59430 2600 160 62190 f2ee libfinderxif.so
64582 2632 184 67398 10746 libftixif.so
24083 1424 48 25555 63d3 libmfeaclientxif.so
59360 1904 280 61544 f068 libmfeaxif.so
20506 1360 48 21914 559a libmld6igmpclientxif.so
69988 2192 328 72508 11b3c libmld6igmpxif.so
105366 3376 344 109086 1aa1e libospfv2xif.so
106649 3416 352 110417 1af51 libospfv3xif.so
372963 5120 1840 379923 5cc13 libpimxif.so
16589 1288 40 17917 45fd libpolicybackendxif.so
18379 1376 32 19787 4d4b libpolicyredist4xif.so
18379 1376 32 19787 4d4b libpolicyredist6xif.so
38126 1688 176 39990 9c36 libpolicyxif.so
15722 1280 32 17034 428a libprofileclientxif.so
23568 1576 96 25240 6298 libprofilexif.so
20220 1360 48 21628 547c libredist4xif.so
20220 1360 48 21628 547c libredist6xif.so
28223 1656 96 29975 7517 libredisttransaction4xif.so
28223 1656 96 29975 7517 libredisttransaction6xif.so
20180 1360 48 21588 5454 libribclientxif.so
127737 3728 416 131881 20329 libribxif.so
120441 3680 360 124481 1e641 libripngxif.so
125995 3712 392 130099 1fc33 libripxif.so
20052 1336 56 21444 53c4 librtrmgrclientxif.so
69988 2984 184 73156 11dc4 librtrmgrxif.so
27404 1680 88 29172 71f4 libsocket4userxif.so
48393 1808 200 50401 c4e1 libsocket4xif.so
27404 1680 88 29172 71f4 libsocket6userxif.so
43895 1784 176 45855 b31f libsocket6xif.so
58097 1648 240 59985 ea51 libstaticroutesxif.so
25799 1408 96 27303 6aa7 libtestpeerxif.so
28323 1784 88 30195 75f3 libtestxif.so
27334 1576 72 28982 7136 libtestxrlsxif.so
55293 2504 152 57949 e25d libvrrpxif.so
Index: xrl/scripts/clnt-gen
===================================================================
--- xrl/scripts/clnt-gen (revision 11574)
+++ xrl/scripts/clnt-gen (working copy)
@@ -57,6 +57,7 @@
def implement_send_xrl(cls, method_no, method, ifqname):
cb_name = "%sCB" % (caps_cpp_classname(method.name()))
+ xrl_ptr_name = "ap_xrl_%s" % method.name()
atypes = ["\n\tconst char*\tdst_xrl_target_name"]
for a in method.args():
atypes.append("\n\tconst %s&\t%s" % (a.cpp_type(), a.name()))
@@ -64,12 +65,13 @@
s = "\nbool\n%s::send_%s(%s\n)\n{\n" \
% (cls, cpp_name(method.name()), csv(atypes))
- s += " static Xrl* x = NULL;\n"
+ s += " Xrl* x = %s.get();\n" % xrl_ptr_name
s += "\n"
s += " if (!x) {\n"
s += " x = new Xrl(dst_xrl_target_name, \"%s\");\n" % ifqname
for a in method.args():
s += " x->args().add(\"%s\", %s);\n" % (a.name(), a.name())
+ s += " %s.reset(x);\n" % xrl_ptr_name
s += " }\n"
s += "\n"
s += " x->set_target(dst_xrl_target_name);\n"
@@ -95,6 +97,11 @@
s += "\n%s);\n\n" % xorp_indent(1)
return s
+def declare_xrl_auto_ptr(method_name):
+ xrl_ptr_name = "ap_xrl_%s" % method_name
+ s = xorp_indent(1) + "auto_ptr<Xrl> %s;\n" % xrl_ptr_name
+ return s
+
def implement_unmarshall(cls, method_no, method):
nargs = []
@@ -163,6 +170,7 @@
#include "libxipc/xrl_error.hh"
#include "libxipc/xrl_sender.hh"
+#include <memory>
""" % (protect(hh_file), protect(hh_file), modulename)
return s
@@ -184,6 +192,12 @@
"""
for i in range(0, len(methods)):
s += declare_unmarshall(methods[i].name())
+
+ s += "private:\n"
+ s += " /* Declare cached Xrl pointers */\n"
+ for i in range(0, len(methods)):
+ s += declare_xrl_auto_ptr(methods[i].name())
+
s += "};\n"
return s
_______________________________________________
Xorp-hackers mailing list
[email protected]
http://mailman.ICSI.Berkeley.EDU/mailman/listinfo/xorp-hackers
