Re: [Update][PATCH v5 7/9] mm/swap: Add cache for swap slots allocation

[Michal Hocko]

On Tue 17-01-17 17:24:15, Chen, Tim C wrote:
> > > +	/*
> > > +	 * Preemption need to be turned on here, because we may sleep
> > > +	 * in refill_swap_slots_cache().  But it is safe, because
> > > +	 * accesses to the per-CPU data structure are protected by a
> > > +	 * mutex.
> > > +	 */
> > 
> > the comment doesn't really explain why it is safe. THere are other users
> > which are not using the lock. E.g. just look at free_swap_slot above.
> > How can
> > 	cache->slots_ret[cache->n_ret++] = entry; be safe wrt.
> > 	pentry = &cache->slots[cache->cur++];
> > 	entry = *pentry;
> > 
> > Both of them might touch the same slot, no? Btw. I would rather prefer this
> > would be a follow up fix with the trace and the detailed explanation.
> > 
> 
> The cache->slots_ret  is protected by cache->free_lock and cache->slots is
> protected by cache->free_lock.

Ohh, I have misread those names and considered them the same thing.
Sorry about the confusion. I will look at code more deeply tomorrow.

> They are two separate structures, one for
> caching the slots returned and one for caching the slots allocated.  So
> they do no touch the same slots.  We'll update the comments so it is clearer.

That would be really appreciated.

-- 
Michal Hocko
SUSE Labs