Opened 7 years ago

Closed 6 years ago

#11476 closed defect (implemented)

Try making all memory pool and freelist code off by default, or clear-on-alloc

Reported by: nickm Owned by: andrea
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay, security, 025-triaged
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We've got mempool.c, and we've got freelists in buf_t. After recent shenanigans, it seems like a good idea to turn them off-by-default to see if any platforms actually need them, and have them on only when needed.

Additionally, we should look to see whether there's any performance hit that matters for making them get cleared before they're released.

Child Tickets

TicketStatusOwnerSummaryComponent
#11623closedtor-0.2.5.4-alpha and git/HEAD fail to build when --disable-buf-freelistsCore Tor/Tor

Change History (16)

comment:1 Changed 6 years ago by nickm

Keywords: 025-triaged added

comment:2 Changed 6 years ago by nickm

Just merged Andrea's "bug11476" branch to 0.2.5.x to fix #11623. As it stood, it only implemented #11623 (fixing --disable-buf-freelists), but that's a good start.

comment:3 Changed 6 years ago by nickm

Current plan on these is to investigate the bug, try to finish writing the patch on each, and then evaluate whether the patch is 0.2.5 or 0.2.6 material in terms of simplicity and importance.

comment:4 Changed 6 years ago by nickm

Owner: set to andrea
Status: newassigned

comment:5 Changed 6 years ago by nickm

Keywords: 024-backport removed

comment:6 Changed 6 years ago by andrea

Query: should I also consider adding a configure toggle for memarea.c/memarea.h in this?

comment:7 Changed 6 years ago by andrea

Hmm, I guess not, since we rely on memarea_drop_all() to free a bunch of stuff at once in routerparse.c and it'd be a huge refactor not to.

comment:8 Changed 6 years ago by andrea

Status: assignedneeds_review

See my completed bug11476 branch; this works well as a relay and client in testing and seems valgrind-clean. We should still do performance comparisons, though.

comment:9 Changed 6 years ago by nickm

Status: needs_reviewneeds_revision

Suggestion:

  • Rather than scattering ENABLE_MEMPOOLS items through our code, why not instead make the code in mempool.h just wrap malloc_zero() and free() when we're running in with mempools disabled? That way, the rest of the code stays pretty.

comment:10 in reply to:  9 Changed 6 years ago by andrea

Replying to nickm:

Suggestion:

  • Rather than scattering ENABLE_MEMPOOLS items through our code, why not instead make the code in mempool.h just wrap malloc_zero() and free() when we're running in with mempools disabled? That way, the rest of the code stays pretty.

Okay, will do.

comment:11 in reply to:  9 Changed 6 years ago by andrea

Replying to nickm:

Suggestion:

  • Rather than scattering ENABLE_MEMPOOLS items through our code, why not instead make the code in mempool.h just wrap malloc_zero() and free() when we're running in with mempools disabled? That way, the rest of the code stays pretty.

...actually, because mp_pool_get() takes a cell pool as an argument, so I'd also have to leave 'static mp_pool_t *cell_pool = NULL;' in relay.c outside the #ifdef if I were keeping the call to mp_pool_get() but making it wrap tor_malloc_zero().

Counter-suggestion: replace mp_pool_get() calls in relay.c with relay_alloc_cell() or something like that, which is #defined to either mp_pool_get(cell_pool) or tor_malloc_zero() as appropriate?

comment:12 Changed 6 years ago by nickm

because mp_pool_get() takes a cell pool as an argument, so I'd also have to leave 'static mp_pool_t *cell_pool = NULL;' in relay.c outside the #ifdef if I were keeping the call to mp_pool_get() but making it wrap tor_malloc_zero().

That's not so bad IMO; you'd just need to have mp_pool_t be semantically "This is a memory pool if ENABLE_MEMPOOLS is set; this is a placeholder object if it isn't."

Counter-suggestion: replace mp_pool_get() calls in relay.c with relay_alloc_cell() or something like that, which is #defined to either mp_pool_get(cell_pool) or tor_malloc_zero() as appropriate?

I think that approach is okay too. The thing I don't want to have happen is multiple instances of

#ifdef ENABLE_MEMPOOLS
  foo();
#else
  bar();
#endif

scattered throughout the codebase. That's a recipe for hard-to-maintain code IMO. Ideally, when there's an ENABLE_foo, its effects should be isolated by a function abstraction at some level, though IMO the exact level is not super critical.

comment:13 in reply to:  12 Changed 6 years ago by andrea

Replying to nickm:

because mp_pool_get() takes a cell pool as an argument, so I'd also have to leave 'static mp_pool_t *cell_pool = NULL;' in relay.c outside the #ifdef if I were keeping the call to mp_pool_get() but making it wrap tor_malloc_zero().

That's not so bad IMO; you'd just need to have mp_pool_t be semantically "This is a memory pool if ENABLE_MEMPOOLS is set; this is a placeholder object if it isn't."

Counter-suggestion: replace mp_pool_get() calls in relay.c with relay_alloc_cell() or something like that, which is #defined to either mp_pool_get(cell_pool) or tor_malloc_zero() as appropriate?

I think that approach is okay too. The thing I don't want to have happen is multiple instances of

#ifdef ENABLE_MEMPOOLS
  foo();
#else
  bar();
#endif

scattered throughout the codebase. That's a recipe for hard-to-maintain code IMO. Ideally, when there's an ENABLE_foo, its effects should be isolated by a function abstraction at some level, though IMO the exact level is not super critical.

Okay, fine by me.

comment:14 in reply to:  12 Changed 6 years ago by andrea

Replying to nickm:

because mp_pool_get() takes a cell pool as an argument, so I'd also have to leave 'static mp_pool_t *cell_pool = NULL;' in relay.c outside the #ifdef if I were keeping the call to mp_pool_get() but making it wrap tor_malloc_zero().

That's not so bad IMO; you'd just need to have mp_pool_t be semantically "This is a memory pool if ENABLE_MEMPOOLS is set; this is a placeholder object if it isn't."

Counter-suggestion: replace mp_pool_get() calls in relay.c with relay_alloc_cell() or something like that, which is #defined to either mp_pool_get(cell_pool) or tor_malloc_zero() as appropriate?

I think that approach is okay too. The thing I don't want to have happen is multiple instances of

#ifdef ENABLE_MEMPOOLS
  foo();
#else
  bar();
#endif

scattered throughout the codebase. That's a recipe for hard-to-maintain code IMO. Ideally, when there's an ENABLE_foo, its effects should be isolated by a function abstraction at some level, though IMO the exact level is not super critical.

Updated branch eliminates the #ifdefs around the alloc/free stuff in relay.c How do you feel about the init_cell_pool()/cleanup_cell_pool() calls in main.c and circuitlist.c?

comment:15 Changed 6 years ago by andrea

Status: needs_revisionneeds_review

comment:16 Changed 6 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Okay; this looks better, and doesn't cause any craziness from coverity or clang-scan. Merged to master.

Note: See TracTickets for help on using tickets.