On Wed, Nov 02, 2016 at 07:08:51AM +0100, Thomas Huth wrote:
> On 31.10.2016 19:51, Andrew Jones wrote:
> > Add some asserts to make sure phys_alloc_init is called only once
> > and before any other alloc calls.
> >
> > Signed-off-by: Andrew Jones <drjones@xxxxxxxxxx>
> > ---
> > lib/alloc.c | 17 +++++++++--------
> > 1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/lib/alloc.c b/lib/alloc.c
> > index e1d7b8a380ff..a83c8c5db850 100644
> > --- a/lib/alloc.c
> > +++ b/lib/alloc.c
> > @@ -42,12 +42,13 @@ void phys_alloc_show(void)
> >
> > void phys_alloc_init(phys_addr_t base_addr, phys_addr_t size)
> > {
> > - spin_lock(&lock);
> > + /* we should only be called once by one processor */
> > + assert(!top);
> > + assert(size);
>
> If you remove the spinlock, isn't there a potential race between the
> assert(!top) and the "top = ..." below? E.g. if two CPUs enter this
> function at the same time, both could check and passs assert(!top)
> independently, and then do the "top = ..." in parallel?
Good point. Adding the asserts is enforcing callers to do the right thing,
so we might as well keep the spinlocks to fully enforce the right thing,
rather than assume callers will mostly do it. I'll also update the
documentation in alloc.h for v2, though, stating that this should only be
called once from the primary cpu, before secondaries are started,
ensuring callers should know what the right thing is.
>
> > base = base_addr;
> > top = base + size;
> > align_min = DEFAULT_MINIMUM_ALIGNMENT;
> > - nr_regions = 0;
> > - spin_unlock(&lock);
> > }
> >
> > void phys_alloc_set_minimum_alignment(phys_addr_t align)
> > @@ -63,14 +64,14 @@ static phys_addr_t phys_alloc_aligned_safe(phys_addr_t size,
> > {
> > static bool warned = false;
> > phys_addr_t addr, size_orig = size;
> > - u64 top_safe;
> > + u64 top_safe = top;
> >
> > - spin_lock(&lock);
> > + if (safe && sizeof(long) == 4)
> > + top_safe = MIN(top, 1ULL << 32);
> >
> > - top_safe = top;
> > + assert(base < top_safe);
> >
> > - if (safe && sizeof(long) == 4)
> > - top_safe = MIN(top_safe, 1ULL << 32);
> > + spin_lock(&lock);
> >
> > align = MAX(align, align_min);
>
> This hunk rather looks like a code re-arrangement to me which is not
> directly related to the patch description - maybe you should put this
> into a separate patch?
Much of it was re-arranging, but as a side-effect to getting the
'assert(base < top_safe)' added in the right place. That assert
ensures the subtraction below is safe and also that base != top_safe,
which would likely mean base == top_safe == 0, i.e. not yet initialized.
Hmm, it should probably be assert(top_safe && base < top_safe) though
to account for the no free memory left case as well.
Thanks,
drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
