Messages in this thread | | | Date | Mon, 25 May 2020 18:50:47 +0200 | From | Sebastian Andrzej Siewior <> | Subject | Re: [PATCH v2 6/7] zram: Allocate struct zcomp_strm as per-CPU memory |
| |
On 2020-05-25 09:24:07 [+0200], Ingo Molnar wrote: > Various typo/spelling fixes: > > > zcomp::stream is a per-CPU pointer, pointing to struct zcomp_strm > > which contains two pointers. Having struct zcomp_strm allocated > > directly as per-CPU memory would avoid one additional memory > > allocation and a pointer dereference. This also simplifies the > > addition of a local_lock to struct zcomp_strm.
thx, updated.
> > diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c > > index 1a8564a79d8dc..ae6dc137a1ed8 100644 > > --- a/drivers/block/zram/zcomp.c > > +++ b/drivers/block/zram/zcomp.c > > @@ -37,19 +37,17 @@ static void zcomp_strm_free(struct zcomp_strm *zstrm) > > if (!IS_ERR_OR_NULL(zstrm->tfm)) > > crypto_free_comp(zstrm->tfm); > > free_pages((unsigned long)zstrm->buffer, 1); > > - kfree(zstrm); > > + zstrm->tfm = NULL; > > + zstrm->buffer = NULL; > > } > > > > /* > > * allocate new zcomp_strm structure with ->tfm initialized by > > * backend, return NULL on error > > */ > > -static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp) > > +static int zcomp_strm_alloc(struct zcomp_strm *zstrm, > > + struct zcomp *comp) > > There's no need to put these into two lines, in a single line it's > only 73 columns long. Leftover from some earlier bloat?
yup, updated.
> > void zcomp_stream_put(struct zcomp *comp) > > @@ -159,16 +157,14 @@ int zcomp_cpu_up_prepare(unsigned int cpu, struct hlist_node *node) > > { > > struct zcomp *comp = hlist_entry(node, struct zcomp, node); > > struct zcomp_strm *zstrm; > > + int ret; > > > > - if (WARN_ON(*per_cpu_ptr(comp->stream, cpu))) > > - return 0; > > - > > - zstrm = zcomp_strm_alloc(comp); > > - if (IS_ERR_OR_NULL(zstrm)) { > > + zstrm = per_cpu_ptr(comp->stream, cpu); > > + ret = zcomp_strm_alloc(zstrm, comp); > > + if (ret) { > > pr_err("Can't allocate a compression stream\n"); > > return -ENOMEM; > > BTW., with the allocation being in a single place and us having a > proper 'ret', the return -ENOMEM could turn into 'return ret'?
yes.
> Thanks, > > Ingo
Sebastian
| |