On Tue, Jan 3, 2017 at 9:33 PM, Timmons C. Player <
[email protected]> wrote:
> If the dynamic memory buffer used for percpu allocations
> wasn't congruent to the requested alignment, then the
> returned memory wouldn't actually be aligned correctly.
> This change forces the bitmap search to start at the proper
> alignment.
>
Thanks, good catch
It appears that the "long align" in dynamic_percpu_buffer was supposed to
provide alignment that would be "enough for anyone" (famous last words...),
but obviously is only enough for 8-byte alignment or less. Did you see this
problem with your own code using dynamic_percpu, or with existing code?
I think we should also drop the "union" and the "long align" stuff in this
patch, which no longer serves any useful purpose?
In malign and friends we have an assert at the end (just before returning)
that "align" worked, if we had it we would have caught this bug more
easily, i.e., something like
assert (!(reinterpret_cast<uintptr_t>(ret) & (alignment - 1)));
Maybe it's worth adding.
Another comment inline below.
>
> Signed-off-by: Timmons C. Player <[email protected]>
> ---
> core/percpu.cc | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/core/percpu.cc b/core/percpu.cc
> index ebccded..920d67e 100644
> --- a/core/percpu.cc
> +++ b/core/percpu.cc
> @@ -29,7 +29,12 @@ size_t dynamic_percpu_base()
> size_t dynamic_percpu_alloc(size_t size, size_t align)
> {
> std::lock_guard<mutex> guard(mtx);
> - for (size_t i = 0; i < dynamic_percpu_max; i += align) {
> +
> + /* Find the first value in the bitmap that has the necessary
> alignment */
> + auto percpu_base = dynamic_percpu_base();
> + auto align_base = ((percpu_base + (align - 1)) & ~(align - 1)) -
> percpu_base;
>
This calculation only works correctly when align is a power of two, so I
suggest that you assert() that it is. You can use the function
is_power_of_two() from <osv/ilog2.hh>.
By the way, for extra clarity you can also use our align_up() macro (from
<osv/align.hh>), which does the same calculation as you did:
auto align_base = align_up(percpu_base, align) - percpu_base
> +
> + for (size_t i = align_base; i < dynamic_percpu_max; i += align) {
> size_t j = 0;
> for (; j < size; ++j) {
> if (dynamic_percpu_allocated.test(i + j)) {
> @@ -54,4 +59,3 @@ void dynamic_percpu_free(size_t offset, size_t size)
> dynamic_percpu_allocated.set(offset + j, false);
> }
> }
> -
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "OSv Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to [email protected].
> For more options, visit https://groups.google.com/d/optout.
>
--
You received this message because you are subscribed to the Google Groups "OSv
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
