On (25/02/06 16:16), Yosry Ahmed wrote:
> > So for spin-lock contention - yes, but that lock really should not
> > be so visible. Other than that we limit the number of compression
> > streams to the number of the CPUs and permit preemption, so it should
> > be the same as the "preemptible per-CPU" streams, roughly.
>
> I think one other problem is that with a pool of streams guarded by a
> single lock all CPUs have to be serialized on that lock, even if there's
> enough streams for all CPUs in theory.
Yes, at the same time it guards list-first-entry, which is not
exceptionally expensive. Yet, somehow, it still showed up on
Kairui's radar.
I think there was also a problem with how on-demand streams were
constructed - GFP_KERNEL allocations from a reclaim path, which
is a tiny bit problematic and deadlock-ish.
> > The difference, perhaps, is that we don't pre-allocate streams, but
> > allocate only as needed. This has two sides: one side is that later
> > allocations can fail, but the other side is that we don't allocate
> > streams that we don't use. Especially secondary streams (priority 1
> > and 2, which are used for recompression). I didn't know it was possible
> > to use per-CPU data and still have preemption enabled at the same time.
> > So I'm not opposed to the idea of still having per-CPU streams and do
> > what zswap folks did.
>
> Note that it's not a free lunch. If preemption is allowed there is
> nothing holding keeping the CPU that you're using its data, and it can
> be offlined. I see that zcomp_cpu_dead() would free the compression
> stream from under its user in this case.
Yes, I took same approach as you did in zswap - we are holding the mutex
that cpu-dead is blocked on as long as the stream is being used.
struct zcomp_strm *zcomp_stream_get(struct zcomp *comp)
{
for (;;) {
struct zcomp_strm *zstrm = raw_cpu_ptr(comp->stream);
/*
* Inspired by zswap
*
* stream is returned with ->mutex locked which prevents
* cpu_dead() from releasing this stream under us, however
* there is still a race window between raw_cpu_ptr() and
* mutex_lock(), during which we could have been migrated
* to a CPU that has already destroyed its stream. If so
* then unlock and re-try on the current CPU.
*/
mutex_lock(&zstrm->lock);
if (likely(zstrm->buffer))
return zstrm;
mutex_unlock(&zstrm->lock);
}
}
void zcomp_stream_put(struct zcomp_strm *zstrm)
{
mutex_unlock(&zstrm->lock);
}
int zcomp_cpu_dead(unsigned int cpu, struct hlist_node *node)
{
struct zcomp *comp = hlist_entry(node, struct zcomp, node);
struct zcomp_strm *zstrm = per_cpu_ptr(comp->stream, cpu);
mutex_lock(&zstrm->lock);
zcomp_strm_free(comp, zstrm);
mutex_unlock(&zstrm->lock);
return 0;
}
> We had a similar problem recently in zswap and it took me a couple of
> iterations to properly fix it. In short, you need to synchronize the CPU
> hotplug callbacks with the users of the compression stream to make sure
> the stream is not freed under the user.
Agreed.
