Opened 2 years ago

Closed 2 years ago

#27208 closed enhancement (wontfix)

add API for allocating aligned memory

Reported by: cyberpunks Owned by:
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: rust-wants rust
Cc: Actual Points:
Parent ID: #23882 Points:
Reviewer: dgoulet Sponsor:

Description

Rust's GlobalAlloc trait can't be implemented without this.

Child Tickets

Change History (9)

comment:1 Changed 2 years ago by cyberpunks

See branch alignedalloc1 at https://gitgud.io/onionk/tor.git

comment:2 Changed 2 years ago by nickm

Status: newneeds_review

comment:3 Changed 2 years ago by chelseakomlo

Keywords: rust added

comment:4 Changed 2 years ago by dgoulet

Status: needs_reviewneeds_revision

Thanks for this!

  • This code uses posix_memalign() as the fallback if both memalign() and aligned_alloc() aren't present. This has been added a _while_ back so I'm not too worried about any system we currently build on today to not have it but hey you never know... (_POSIX_C_SOURCE >= 200112L).

Should we add a check at configure time and fail if none of the 3 functions are present?

  • I would add a PREDICT_UNLIKELY() around the posix_memalign() for sake of "performance" :).
  • I think we should make static void *aligned_alloc... inline in my opinion... might need some code change from the patch but since tor_malloc() is used extensively, any performance gain is good.
  • More a meta-physic question: Do we really need to reroute every tor_malloc() to the aligned one with alignment set to 1? In the case of memalign() and aligned_alloc(), of what I can read from the glibc, it will be re-routed to the normal malloc() if alignment is below some minimum.

But with posix_memalign(), this means that every tor_malloc() now needs to do a MAX(), which results in _always_ using sizeof(void *). I know this is very cheap CPU side but maybe we can avoid this couple of extra cycles by simply making tor_malloc_() use the right alignment by default and removing the MAX() ?

I know I'm arguing about nanoseconds here but tor_malloc() is one of the *core* foundation of the entire code base, adding any extra work to it is something I personally want to avoid unless it is security related.

comment:5 Changed 2 years ago by dgoulet

Reviewer: dgoulet

comment:6 Changed 2 years ago by nickm

It might be incorrect to treat malloc(sz) as meaning aligned_malloc(sz,1). malloc() is required to return memory that is aligned suitably for any built-in type, whereas aligned_malloc(sz,1) doesn't have that requirement, FWICT.

comment:7 Changed 2 years ago by nickm

From personal communication from a Rust dev: unless explictly told to do so in rust, Rust won't try to call the allocator with an alignment greater than the pointer size. So malloc() should work find, given malloc()'s guarantees.

comment:8 Changed 2 years ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

comment:9 Changed 2 years ago by dgoulet

Resolution: wontfix
Status: needs_revisionclosed

From comment:7, I'll consider this a "wontfix". Feel free to re-open if you don't agree or we end up needing this code.

Note: See TracTickets for help on using tickets.