[PATCH v2] lib/slab: introduce sl_allocz() function and use it in Babel
The babel protocol code was initialising objects returned from the slab allocator by assigning to each of the struct members individually, but wasn't touching the NODE member while doing so. This leads to warnings on debug builds since commit: baac7009063d ("List expensive check.") To fix this, introduce an sl_allocz() variant of the slab allocator which will zero out the memory before returning it, and switch all the babel call sites to use this version. The overhead for doing this should be negligible for small objects, and in the case of babel, the largest object being allocated was being zeroed anyway, so we can drop the memset in babel_read_tlv(). Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk> --- v2: - Introduce sl_allocz() instead of adding more zeroing to the babel code. lib/resource.h | 1 + lib/slab.c | 24 ++++++++++++++++++++++++ proto/babel/babel.c | 7 +++---- proto/babel/packets.c | 7 +++---- 4 files changed, 31 insertions(+), 8 deletions(-) diff --git a/lib/resource.h b/lib/resource.h index ad17d9ed4..b56bcff55 100644 --- a/lib/resource.h +++ b/lib/resource.h @@ -83,6 +83,7 @@ typedef struct slab slab; slab *sl_new(pool *, unsigned size); void *sl_alloc(slab *); +void *sl_allocz(slab *); void sl_free(slab *, void *); /* diff --git a/lib/slab.c b/lib/slab.c index c3035b45b..f31355e02 100644 --- a/lib/slab.c +++ b/lib/slab.c @@ -88,6 +88,14 @@ sl_alloc(slab *s) return o->data; } +void * +sl_allocz(slab *s) +{ + void *obj = sl_alloc(s); + memset(obj, 0, s->size); + return obj; +} + void sl_free(slab *s, void *oo) { @@ -278,6 +286,22 @@ no_partial: goto okay; } +/** + * sl_allocz - allocate an object from Slab and zero it + * @s: slab + * + * sl_allocz() allocates space for a single object from the + * Slab and returns a pointer to the object after zeroing out + * the object memory. + */ +void * +sl_allocz(slab *s) +{ + void *obj = sl_alloc(s); + memset(obj, 0, s->data_size); + return obj; +} + /** * sl_free - return a free object back to a Slab * @s: slab diff --git a/proto/babel/babel.c b/proto/babel/babel.c index 618abaa8a..4b6b9d7f9 100644 --- a/proto/babel/babel.c +++ b/proto/babel/babel.c @@ -113,7 +113,7 @@ babel_get_source(struct babel_proto *p, struct babel_entry *e, u64 router_id) if (s) return s; - s = sl_alloc(p->source_slab); + s = sl_allocz(p->source_slab); s->router_id = router_id; s->expires = current_time() + BABEL_GARBAGE_INTERVAL; s->seqno = 0; @@ -159,8 +159,7 @@ babel_get_route(struct babel_proto *p, struct babel_entry *e, struct babel_neigh if (r) return r; - r = sl_alloc(p->route_slab); - memset(r, 0, sizeof(*r)); + r = sl_allocz(p->route_slab); r->e = e; r->neigh = nbr; @@ -323,7 +322,7 @@ babel_add_seqno_request(struct babel_proto *p, struct babel_entry *e, } /* No entries found */ - sr = sl_alloc(p->seqno_slab); + sr = sl_allocz(p->seqno_slab); found: sr->router_id = router_id; diff --git a/proto/babel/packets.c b/proto/babel/packets.c index d4ecf649d..e135866f4 100644 --- a/proto/babel/packets.c +++ b/proto/babel/packets.c @@ -1144,7 +1144,6 @@ babel_read_tlv(struct babel_tlv *hdr, return PARSE_ERROR; state->current_tlv_endpos = tlv_data[hdr->type].min_length; - memset(msg, 0, sizeof(*msg)); int res = tlv_data[hdr->type].read_tlv(hdr, msg, state); if (res != PARSE_SUCCESS) @@ -1275,7 +1274,7 @@ void babel_send_unicast(union babel_msg *msg, struct babel_iface *ifa, ip_addr dest) { struct babel_proto *p = ifa->proto; - struct babel_msg_node *msgn = sl_alloc(p->msg_slab); + struct babel_msg_node *msgn = sl_allocz(p->msg_slab); list queue; msgn->msg = *msg; @@ -1304,7 +1303,7 @@ void babel_enqueue(union babel_msg *msg, struct babel_iface *ifa) { struct babel_proto *p = ifa->proto; - struct babel_msg_node *msgn = sl_alloc(p->msg_slab); + struct babel_msg_node *msgn = sl_allocz(p->msg_slab); msgn->msg = *msg; add_tail(&ifa->msg_queue, NODE msgn); babel_kick_queue(ifa); @@ -1386,7 +1385,7 @@ babel_process_packet(struct babel_pkt_header *pkt, int len, break; } - msg = sl_alloc(p->msg_slab); + msg = sl_allocz(p->msg_slab); res = babel_read_tlv(tlv, &msg->msg, &state); if (res == PARSE_SUCCESS) { -- 2.29.2
On 11:07 23.11.20, Toke Høiland-Jørgensen wrote:
+void * +sl_allocz(slab *s) +{ + void *obj = sl_alloc(s); + memset(obj, 0, s->size); + return obj; +} +
This is the same function name and (almost same) implementation as further down in this patch. Did you forget to remove it?
void sl_free(slab *s, void *oo) { @@ -278,6 +286,22 @@ no_partial: goto okay; }
+/** + * sl_allocz - allocate an object from Slab and zero it + * @s: slab + * + * sl_allocz() allocates space for a single object from the + * Slab and returns a pointer to the object after zeroing out + * the object memory. + */ +void * +sl_allocz(slab *s) +{ + void *obj = sl_alloc(s); + memset(obj, 0, s->data_size); + return obj; +} +
^ second copy of the function but it is using s->data_size instead.
Andreas Rammhold <andreas@rammhold.de> writes:
On 11:07 23.11.20, Toke Høiland-Jørgensen wrote:
+void * +sl_allocz(slab *s) +{ + void *obj = sl_alloc(s); + memset(obj, 0, s->size); + return obj; +} +
This is the same function name and (almost same) implementation as further down in this patch. Did you forget to remove it?
No, look at the ifdefs - this one is for the FAKE_SLAB case :) -Toke
On 11:33 23.11.20, Toke Høiland-Jørgensen wrote:
Andreas Rammhold <andreas@rammhold.de> writes:
On 11:07 23.11.20, Toke Høiland-Jørgensen wrote:
+void * +sl_allocz(slab *s) +{ + void *obj = sl_alloc(s); + memset(obj, 0, s->size); + return obj; +} +
This is the same function name and (almost same) implementation as further down in this patch. Did you forget to remove it?
No, look at the ifdefs - this one is for the FAKE_SLAB case :)
Ok, I got confused by the mail highlighting. In a proper editor it is more obvious. andi-
Andreas Rammhold <andreas@rammhold.de> writes:
On 11:33 23.11.20, Toke Høiland-Jørgensen wrote:
Andreas Rammhold <andreas@rammhold.de> writes:
On 11:07 23.11.20, Toke Høiland-Jørgensen wrote:
+void * +sl_allocz(slab *s) +{ + void *obj = sl_alloc(s); + memset(obj, 0, s->size); + return obj; +} +
This is the same function name and (almost same) implementation as further down in this patch. Did you forget to remove it?
No, look at the ifdefs - this one is for the FAKE_SLAB case :)
Ok, I got confused by the mail highlighting. In a proper editor it is more obvious.
Hehe, yeah, I often miss those ifdef sections entirely and end up sending out patches that don't compile at all if the config is wrong :) -Toke
While testing this patch I asked myself if there are actually still valid users of non-zeroed memory and if we shouldn't just change the default to be zeroed? That would also remove the need to patch every single call site. andi-
andreas@rammhold.de writes:
While testing this patch I asked myself if there are actually still valid users of non-zeroed memory and if we shouldn't just change the default to be zeroed? That would also remove the need to patch every single call site.
Well there are other callers that already perform manual clearing, so just changing sl_alloc() would lead to double-clearing there. Not a huge issue, I suppose, but I'll leave it up to the maintainers to decide :) -Toke
On 11/23/20 1:45 PM, Toke Høiland-Jørgensen wrote:
andreas@rammhold.de writes:
While testing this patch I asked myself if there are actually still valid users of non-zeroed memory and if we shouldn't just change the default to be zeroed? That would also remove the need to patch every single call site.
Well there are other callers that already perform manual clearing, so just changing sl_alloc() would lead to double-clearing there. Not a huge issue, I suppose, but I'll leave it up to the maintainers to decide :)
An allocated structure from slab is typically intended to be used in hotpaths with fast alloc and dealloc. After going multithreaded, it may have some impact on performance. I think this is more of a documentation issue, rather than a reason to introduce double-clearing. Maria
On Mon, Nov 23, 2020 at 11:07:04AM +0100, Toke Høiland-Jørgensen wrote:
The babel protocol code was initialising objects returned from the slab allocator by assigning to each of the struct members individually, but wasn't touching the NODE member while doing so. This leads to warnings on debug builds since commit:
baac7009063d ("List expensive check.")
Thanks, merged. Actually, i used few parts from v1. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
Ondrej Zajicek <santiago@crfreenet.org> writes:
On Mon, Nov 23, 2020 at 11:07:04AM +0100, Toke Høiland-Jørgensen wrote:
The babel protocol code was initialising objects returned from the slab allocator by assigning to each of the struct members individually, but wasn't touching the NODE member while doing so. This leads to warnings on debug builds since commit:
baac7009063d ("List expensive check.")
Thanks, merged.
Actually, i used few parts from v1.
Right, cool, LGTM :) -Toke
participants (5)
-
Andreas Rammhold -
andreas＠rammhold.de
-
Maria Matejka -
Ondrej Zajicek -
Toke Høiland-Jørgensen