We waste sizeof(swp_entry_t) for zswap header when using zsmalloc
as zpool driver because zsmalloc doesn't support eviction.
Add zpool_shrinkable() to detect if zpool is shrinkable, and use
it in zswap to avoid waste memory for zswap header.
Signed-off-by: Yu Zhao <[email protected]>
---
include/linux/zpool.h | 2 ++
mm/zpool.c | 17 ++++++++++++++++-
mm/zsmalloc.c | 7 -------
mm/zswap.c | 20 ++++++++++----------
4 files changed, 28 insertions(+), 18 deletions(-)
diff --git a/include/linux/zpool.h b/include/linux/zpool.h
index 004ba807df96..3f0ac2ab74aa 100644
--- a/include/linux/zpool.h
+++ b/include/linux/zpool.h
@@ -108,4 +108,6 @@ void zpool_register_driver(struct zpool_driver *driver);
int zpool_unregister_driver(struct zpool_driver *driver);
+bool zpool_shrinkable(struct zpool *pool);
+
#endif
diff --git a/mm/zpool.c b/mm/zpool.c
index fd3ff719c32c..839d4234c540 100644
--- a/mm/zpool.c
+++ b/mm/zpool.c
@@ -296,7 +296,8 @@ void zpool_free(struct zpool *zpool, unsigned long handle)
int zpool_shrink(struct zpool *zpool, unsigned int pages,
unsigned int *reclaimed)
{
- return zpool->driver->shrink(zpool->pool, pages, reclaimed);
+ return zpool_shrinkable(zpool) ?
+ zpool->driver->shrink(zpool->pool, pages, reclaimed) : -EINVAL;
}
/**
@@ -355,6 +356,20 @@ u64 zpool_get_total_size(struct zpool *zpool)
return zpool->driver->total_size(zpool->pool);
}
+/**
+ * zpool_shrinkable() - Test if zpool is shrinkable
+ * @pool The zpool to test
+ *
+ * Zpool is only shrinkable when it's created with struct
+ * zpool_ops.evict and its driver implements struct zpool_driver.shrink.
+ *
+ * Returns: true if shrinkable; false otherwise.
+ */
+bool zpool_shrinkable(struct zpool *zpool)
+{
+ return zpool->ops && zpool->ops->evict && zpool->driver->shrink;
+}
+
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Dan Streetman <[email protected]>");
MODULE_DESCRIPTION("Common API for compressed memory storage");
diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 683c0651098c..9cc741bcdb32 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -407,12 +407,6 @@ static void zs_zpool_free(void *pool, unsigned long handle)
zs_free(pool, handle);
}
-static int zs_zpool_shrink(void *pool, unsigned int pages,
- unsigned int *reclaimed)
-{
- return -EINVAL;
-}
-
static void *zs_zpool_map(void *pool, unsigned long handle,
enum zpool_mapmode mm)
{
@@ -450,7 +444,6 @@ static struct zpool_driver zs_zpool_driver = {
.destroy = zs_zpool_destroy,
.malloc = zs_zpool_malloc,
.free = zs_zpool_free,
- .shrink = zs_zpool_shrink,
.map = zs_zpool_map,
.unmap = zs_zpool_unmap,
.total_size = zs_zpool_total_size,
diff --git a/mm/zswap.c b/mm/zswap.c
index d39581a076c3..15d2ea29a6fa 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -964,11 +964,11 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
struct zswap_entry *entry, *dupentry;
struct crypto_comp *tfm;
int ret;
- unsigned int dlen = PAGE_SIZE, len;
+ unsigned int hlen, dlen = PAGE_SIZE;
unsigned long handle;
char *buf;
u8 *src, *dst;
- struct zswap_header *zhdr;
+ struct zswap_header zhdr = { .swpentry = swp_entry(type, offset) };
if (!zswap_enabled || !tree) {
ret = -ENODEV;
@@ -1013,8 +1013,8 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
}
/* store */
- len = dlen + sizeof(struct zswap_header);
- ret = zpool_malloc(entry->pool->zpool, len,
+ hlen = zpool_shrinkable(entry->pool->zpool) ? sizeof(zhdr) : 0;
+ ret = zpool_malloc(entry->pool->zpool, hlen + dlen,
__GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM,
&handle);
if (ret == -ENOSPC) {
@@ -1025,10 +1025,9 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
zswap_reject_alloc_fail++;
goto put_dstmem;
}
- zhdr = zpool_map_handle(entry->pool->zpool, handle, ZPOOL_MM_RW);
- zhdr->swpentry = swp_entry(type, offset);
- buf = (u8 *)(zhdr + 1);
- memcpy(buf, dst, dlen);
+ buf = zpool_map_handle(entry->pool->zpool, handle, ZPOOL_MM_RW);
+ memcpy(buf, &zhdr, hlen);
+ memcpy(buf + hlen, dst, dlen);
zpool_unmap_handle(entry->pool->zpool, handle);
put_cpu_var(zswap_dstmem);
@@ -1091,8 +1090,9 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset,
/* decompress */
dlen = PAGE_SIZE;
- src = (u8 *)zpool_map_handle(entry->pool->zpool, entry->handle,
- ZPOOL_MM_RO) + sizeof(struct zswap_header);
+ src = zpool_map_handle(entry->pool->zpool, entry->handle, ZPOOL_MM_RO);
+ if (zpool_shrinkable(entry->pool->zpool))
+ src += sizeof(struct zswap_header);
dst = kmap_atomic(page);
tfm = *get_cpu_ptr(entry->pool->tfm);
ret = crypto_comp_decompress(tfm, src, entry->length, dst, &dlen);
--
2.16.0.rc0.223.g4a4ac83678-goog
On (01/08/18 14:51), Yu Zhao wrote:
> We waste sizeof(swp_entry_t) for zswap header when using zsmalloc
> as zpool driver because zsmalloc doesn't support eviction.
>
> Add zpool_shrinkable() to detect if zpool is shrinkable, and use
> it in zswap to avoid waste memory for zswap header.
>
> Signed-off-by: Yu Zhao <[email protected]>
at a glance, looks good to me
Reviewed-by: Sergey Senozhatsky <[email protected]>
-ss
On (01/08/18 14:51), Yu Zhao wrote:
[..]
> int zpool_shrink(struct zpool *zpool, unsigned int pages,
> unsigned int *reclaimed)
> {
> - return zpool->driver->shrink(zpool->pool, pages, reclaimed);
> + return zpool_shrinkable(zpool) ?
> + zpool->driver->shrink(zpool->pool, pages, reclaimed) : -EINVAL;
> }
>
> /**
> @@ -355,6 +356,20 @@ u64 zpool_get_total_size(struct zpool *zpool)
> return zpool->driver->total_size(zpool->pool);
> }
>
> +/**
> + * zpool_shrinkable() - Test if zpool is shrinkable
> + * @pool The zpool to test
> + *
> + * Zpool is only shrinkable when it's created with struct
> + * zpool_ops.evict and its driver implements struct zpool_driver.shrink.
> + *
> + * Returns: true if shrinkable; false otherwise.
> + */
> +bool zpool_shrinkable(struct zpool *zpool)
> +{
> + return zpool->ops && zpool->ops->evict && zpool->driver->shrink;
> +}
just a side note,
it might be a bit confusing and maybe there is a better
name for it. zsmalloc is shrinkable (we register a shrinker
callback), but not in the way zpool defines it.
-ss
On Tue, Jan 09, 2018 at 01:48:17PM +0900, Sergey Senozhatsky wrote:
> On (01/08/18 14:51), Yu Zhao wrote:
> [..]
> > int zpool_shrink(struct zpool *zpool, unsigned int pages,
> > unsigned int *reclaimed)
> > {
> > - return zpool->driver->shrink(zpool->pool, pages, reclaimed);
> > + return zpool_shrinkable(zpool) ?
> > + zpool->driver->shrink(zpool->pool, pages, reclaimed) : -EINVAL;
> > }
> >
> > /**
> > @@ -355,6 +356,20 @@ u64 zpool_get_total_size(struct zpool *zpool)
> > return zpool->driver->total_size(zpool->pool);
> > }
> >
> > +/**
> > + * zpool_shrinkable() - Test if zpool is shrinkable
> > + * @pool The zpool to test
> > + *
> > + * Zpool is only shrinkable when it's created with struct
> > + * zpool_ops.evict and its driver implements struct zpool_driver.shrink.
> > + *
> > + * Returns: true if shrinkable; false otherwise.
> > + */
> > +bool zpool_shrinkable(struct zpool *zpool)
> > +{
> > + return zpool->ops && zpool->ops->evict && zpool->driver->shrink;
> > +}
>
> just a side note,
> it might be a bit confusing and maybe there is a better
> name for it. zsmalloc is shrinkable (we register a shrinker
> callback), but not in the way zpool defines it.
Thanks. Do zpool_evictable() and zpool->driver->evict make more sense?
On Mon, Jan 8, 2018 at 5:51 PM, Yu Zhao <[email protected]> wrote:
> We waste sizeof(swp_entry_t) for zswap header when using zsmalloc
> as zpool driver because zsmalloc doesn't support eviction.
>
> Add zpool_shrinkable() to detect if zpool is shrinkable, and use
> it in zswap to avoid waste memory for zswap header.
>
> Signed-off-by: Yu Zhao <[email protected]>
> ---
> include/linux/zpool.h | 2 ++
> mm/zpool.c | 17 ++++++++++++++++-
> mm/zsmalloc.c | 7 -------
> mm/zswap.c | 20 ++++++++++----------
> 4 files changed, 28 insertions(+), 18 deletions(-)
>
> diff --git a/include/linux/zpool.h b/include/linux/zpool.h
> index 004ba807df96..3f0ac2ab74aa 100644
> --- a/include/linux/zpool.h
> +++ b/include/linux/zpool.h
> @@ -108,4 +108,6 @@ void zpool_register_driver(struct zpool_driver *driver);
>
> int zpool_unregister_driver(struct zpool_driver *driver);
>
> +bool zpool_shrinkable(struct zpool *pool);
> +
> #endif
> diff --git a/mm/zpool.c b/mm/zpool.c
> index fd3ff719c32c..839d4234c540 100644
> --- a/mm/zpool.c
> +++ b/mm/zpool.c
> @@ -296,7 +296,8 @@ void zpool_free(struct zpool *zpool, unsigned long handle)
> int zpool_shrink(struct zpool *zpool, unsigned int pages,
> unsigned int *reclaimed)
> {
> - return zpool->driver->shrink(zpool->pool, pages, reclaimed);
> + return zpool_shrinkable(zpool) ?
> + zpool->driver->shrink(zpool->pool, pages, reclaimed) : -EINVAL;
> }
>
> /**
> @@ -355,6 +356,20 @@ u64 zpool_get_total_size(struct zpool *zpool)
> return zpool->driver->total_size(zpool->pool);
> }
>
> +/**
> + * zpool_shrinkable() - Test if zpool is shrinkable
> + * @pool The zpool to test
> + *
> + * Zpool is only shrinkable when it's created with struct
> + * zpool_ops.evict and its driver implements struct zpool_driver.shrink.
> + *
> + * Returns: true if shrinkable; false otherwise.
> + */
> +bool zpool_shrinkable(struct zpool *zpool)
> +{
> + return zpool->ops && zpool->ops->evict && zpool->driver->shrink;
as these things won't ever change for the life of the zpool, it would
probably be better to just check them at zpool creation time and set a
single new zpool param, like 'zpool->shrinkable'. since this function
will be called for every page that's swapped in or out, that may save
a bit of time.
also re: calling it 'shrinkable' or 'evictable', the real thing zswap
is interested in is if it needs to include the header info that
zswap_writeback_entry (i.e. ops->evict) later needs, so yeah it does
make more sense to call it zpool_evictable() and zpool->evictable.
However, I think the function should still be zpool_shrink() and
zpool->driver->shrink(), because it should be possible for
zs_pool_shrink() to call the normal zsmalloc shrinker, instead of
doing the zswap-style eviction, even if it doesn't do that currently.
> +}
> +
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("Dan Streetman <[email protected]>");
> MODULE_DESCRIPTION("Common API for compressed memory storage");
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index 683c0651098c..9cc741bcdb32 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -407,12 +407,6 @@ static void zs_zpool_free(void *pool, unsigned long handle)
> zs_free(pool, handle);
> }
>
> -static int zs_zpool_shrink(void *pool, unsigned int pages,
> - unsigned int *reclaimed)
> -{
> - return -EINVAL;
> -}
> -
> static void *zs_zpool_map(void *pool, unsigned long handle,
> enum zpool_mapmode mm)
> {
> @@ -450,7 +444,6 @@ static struct zpool_driver zs_zpool_driver = {
> .destroy = zs_zpool_destroy,
> .malloc = zs_zpool_malloc,
> .free = zs_zpool_free,
> - .shrink = zs_zpool_shrink,
> .map = zs_zpool_map,
> .unmap = zs_zpool_unmap,
> .total_size = zs_zpool_total_size,
> diff --git a/mm/zswap.c b/mm/zswap.c
> index d39581a076c3..15d2ea29a6fa 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -964,11 +964,11 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
> struct zswap_entry *entry, *dupentry;
> struct crypto_comp *tfm;
> int ret;
> - unsigned int dlen = PAGE_SIZE, len;
> + unsigned int hlen, dlen = PAGE_SIZE;
> unsigned long handle;
> char *buf;
> u8 *src, *dst;
> - struct zswap_header *zhdr;
> + struct zswap_header zhdr = { .swpentry = swp_entry(type, offset) };
>
> if (!zswap_enabled || !tree) {
> ret = -ENODEV;
> @@ -1013,8 +1013,8 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
> }
>
> /* store */
> - len = dlen + sizeof(struct zswap_header);
> - ret = zpool_malloc(entry->pool->zpool, len,
> + hlen = zpool_shrinkable(entry->pool->zpool) ? sizeof(zhdr) : 0;
> + ret = zpool_malloc(entry->pool->zpool, hlen + dlen,
> __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM,
> &handle);
> if (ret == -ENOSPC) {
> @@ -1025,10 +1025,9 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
> zswap_reject_alloc_fail++;
> goto put_dstmem;
> }
> - zhdr = zpool_map_handle(entry->pool->zpool, handle, ZPOOL_MM_RW);
> - zhdr->swpentry = swp_entry(type, offset);
> - buf = (u8 *)(zhdr + 1);
> - memcpy(buf, dst, dlen);
> + buf = zpool_map_handle(entry->pool->zpool, handle, ZPOOL_MM_RW);
> + memcpy(buf, &zhdr, hlen);
> + memcpy(buf + hlen, dst, dlen);
> zpool_unmap_handle(entry->pool->zpool, handle);
> put_cpu_var(zswap_dstmem);
>
> @@ -1091,8 +1090,9 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset,
>
> /* decompress */
> dlen = PAGE_SIZE;
> - src = (u8 *)zpool_map_handle(entry->pool->zpool, entry->handle,
> - ZPOOL_MM_RO) + sizeof(struct zswap_header);
> + src = zpool_map_handle(entry->pool->zpool, entry->handle, ZPOOL_MM_RO);
> + if (zpool_shrinkable(entry->pool->zpool))
> + src += sizeof(struct zswap_header);
> dst = kmap_atomic(page);
> tfm = *get_cpu_ptr(entry->pool->tfm);
> ret = crypto_comp_decompress(tfm, src, entry->length, dst, &dlen);
> --
> 2.16.0.rc0.223.g4a4ac83678-goog
>
On Tue, Jan 09, 2018 at 01:25:18PM -0500, Dan Streetman wrote:
> On Mon, Jan 8, 2018 at 5:51 PM, Yu Zhao <[email protected]> wrote:
> > We waste sizeof(swp_entry_t) for zswap header when using zsmalloc
> > as zpool driver because zsmalloc doesn't support eviction.
> >
> > Add zpool_shrinkable() to detect if zpool is shrinkable, and use
> > it in zswap to avoid waste memory for zswap header.
> >
> > Signed-off-by: Yu Zhao <[email protected]>
> > ---
> > include/linux/zpool.h | 2 ++
> > mm/zpool.c | 17 ++++++++++++++++-
> > mm/zsmalloc.c | 7 -------
> > mm/zswap.c | 20 ++++++++++----------
> > 4 files changed, 28 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/linux/zpool.h b/include/linux/zpool.h
> > index 004ba807df96..3f0ac2ab74aa 100644
> > --- a/include/linux/zpool.h
> > +++ b/include/linux/zpool.h
> > @@ -108,4 +108,6 @@ void zpool_register_driver(struct zpool_driver *driver);
> >
> > int zpool_unregister_driver(struct zpool_driver *driver);
> >
> > +bool zpool_shrinkable(struct zpool *pool);
> > +
> > #endif
> > diff --git a/mm/zpool.c b/mm/zpool.c
> > index fd3ff719c32c..839d4234c540 100644
> > --- a/mm/zpool.c
> > +++ b/mm/zpool.c
> > @@ -296,7 +296,8 @@ void zpool_free(struct zpool *zpool, unsigned long handle)
> > int zpool_shrink(struct zpool *zpool, unsigned int pages,
> > unsigned int *reclaimed)
> > {
> > - return zpool->driver->shrink(zpool->pool, pages, reclaimed);
> > + return zpool_shrinkable(zpool) ?
> > + zpool->driver->shrink(zpool->pool, pages, reclaimed) : -EINVAL;
> > }
> >
> > /**
> > @@ -355,6 +356,20 @@ u64 zpool_get_total_size(struct zpool *zpool)
> > return zpool->driver->total_size(zpool->pool);
> > }
> >
> > +/**
> > + * zpool_shrinkable() - Test if zpool is shrinkable
> > + * @pool The zpool to test
> > + *
> > + * Zpool is only shrinkable when it's created with struct
> > + * zpool_ops.evict and its driver implements struct zpool_driver.shrink.
> > + *
> > + * Returns: true if shrinkable; false otherwise.
> > + */
> > +bool zpool_shrinkable(struct zpool *zpool)
> > +{
> > + return zpool->ops && zpool->ops->evict && zpool->driver->shrink;
>
> as these things won't ever change for the life of the zpool, it would
> probably be better to just check them at zpool creation time and set a
> single new zpool param, like 'zpool->shrinkable'. since this function
> will be called for every page that's swapped in or out, that may save
> a bit of time.
Ack.
> also re: calling it 'shrinkable' or 'evictable', the real thing zswap
> is interested in is if it needs to include the header info that
> zswap_writeback_entry (i.e. ops->evict) later needs, so yeah it does
> make more sense to call it zpool_evictable() and zpool->evictable.
> However, I think the function should still be zpool_shrink() and
> zpool->driver->shrink(), because it should be possible for
> zs_pool_shrink() to call the normal zsmalloc shrinker, instead of
> doing the zswap-style eviction, even if it doesn't do that currently.
I agree we keep zpool_shrink(). It could either shrink pool if driver
supports slab shrinker by providing zpool->driver->shrink or evict
pages from pool if driver supports zpool->driver->evict (which in turn
calls ops->evict provided by zswap) or both.
We can't use a single zpool->driver->callback to achieve both because
there will be no way for zswap to know if driver uses ops->evict thus
no way to determine if zswap_header is needed.
So for now, I think it'd be better if we deleted zpool->driver->shrink
from zsmalloc and renamed it to zpool->driver->evict in zbud. Later
if we decide zpool_shrink should also call zsmalloc slab shrinker, we
add a new callback.
On Tue, Jan 9, 2018 at 5:47 PM, Yu Zhao <[email protected]> wrote:
> On Tue, Jan 09, 2018 at 01:25:18PM -0500, Dan Streetman wrote:
>> On Mon, Jan 8, 2018 at 5:51 PM, Yu Zhao <[email protected]> wrote:
>> > We waste sizeof(swp_entry_t) for zswap header when using zsmalloc
>> > as zpool driver because zsmalloc doesn't support eviction.
>> >
>> > Add zpool_shrinkable() to detect if zpool is shrinkable, and use
>> > it in zswap to avoid waste memory for zswap header.
>> >
>> > Signed-off-by: Yu Zhao <[email protected]>
>> > ---
>> > include/linux/zpool.h | 2 ++
>> > mm/zpool.c | 17 ++++++++++++++++-
>> > mm/zsmalloc.c | 7 -------
>> > mm/zswap.c | 20 ++++++++++----------
>> > 4 files changed, 28 insertions(+), 18 deletions(-)
>> >
>> > diff --git a/include/linux/zpool.h b/include/linux/zpool.h
>> > index 004ba807df96..3f0ac2ab74aa 100644
>> > --- a/include/linux/zpool.h
>> > +++ b/include/linux/zpool.h
>> > @@ -108,4 +108,6 @@ void zpool_register_driver(struct zpool_driver *driver);
>> >
>> > int zpool_unregister_driver(struct zpool_driver *driver);
>> >
>> > +bool zpool_shrinkable(struct zpool *pool);
>> > +
>> > #endif
>> > diff --git a/mm/zpool.c b/mm/zpool.c
>> > index fd3ff719c32c..839d4234c540 100644
>> > --- a/mm/zpool.c
>> > +++ b/mm/zpool.c
>> > @@ -296,7 +296,8 @@ void zpool_free(struct zpool *zpool, unsigned long handle)
>> > int zpool_shrink(struct zpool *zpool, unsigned int pages,
>> > unsigned int *reclaimed)
>> > {
>> > - return zpool->driver->shrink(zpool->pool, pages, reclaimed);
>> > + return zpool_shrinkable(zpool) ?
>> > + zpool->driver->shrink(zpool->pool, pages, reclaimed) : -EINVAL;
>> > }
>> >
>> > /**
>> > @@ -355,6 +356,20 @@ u64 zpool_get_total_size(struct zpool *zpool)
>> > return zpool->driver->total_size(zpool->pool);
>> > }
>> >
>> > +/**
>> > + * zpool_shrinkable() - Test if zpool is shrinkable
>> > + * @pool The zpool to test
>> > + *
>> > + * Zpool is only shrinkable when it's created with struct
>> > + * zpool_ops.evict and its driver implements struct zpool_driver.shrink.
>> > + *
>> > + * Returns: true if shrinkable; false otherwise.
>> > + */
>> > +bool zpool_shrinkable(struct zpool *zpool)
>> > +{
>> > + return zpool->ops && zpool->ops->evict && zpool->driver->shrink;
>>
>> as these things won't ever change for the life of the zpool, it would
>> probably be better to just check them at zpool creation time and set a
>> single new zpool param, like 'zpool->shrinkable'. since this function
>> will be called for every page that's swapped in or out, that may save
>> a bit of time.
>
> Ack.
>
>> also re: calling it 'shrinkable' or 'evictable', the real thing zswap
>> is interested in is if it needs to include the header info that
>> zswap_writeback_entry (i.e. ops->evict) later needs, so yeah it does
>> make more sense to call it zpool_evictable() and zpool->evictable.
>> However, I think the function should still be zpool_shrink() and
>> zpool->driver->shrink(), because it should be possible for
>> zs_pool_shrink() to call the normal zsmalloc shrinker, instead of
>> doing the zswap-style eviction, even if it doesn't do that currently.
>
> I agree we keep zpool_shrink(). It could either shrink pool if driver
> supports slab shrinker by providing zpool->driver->shrink or evict
> pages from pool if driver supports zpool->driver->evict (which in turn
> calls ops->evict provided by zswap) or both.
>
> We can't use a single zpool->driver->callback to achieve both because
> there will be no way for zswap to know if driver uses ops->evict thus
> no way to determine if zswap_header is needed.
>
> So for now, I think it'd be better if we deleted zpool->driver->shrink
> from zsmalloc and renamed it to zpool->driver->evict in zbud. Later
> if we decide zpool_shrink should also call zsmalloc slab shrinker, we
> add a new callback.
Well, I think shrink vs evict an implementation detail, isn't it?
That is, from zswap's perspective, there should be:
zpool_evictable()
if true, zswap needs to include the header on each compressed page,
because the zpool may callback zpool->ops->evict() which calls
zswap_writeback_entry() which expects the entry to start with a zswap
header.
if false, zswap doesn't need to include the header, because the zpool
will never, ever call zpool->ops->evict
zpool_shrink()
this will try to shrink the zpool, using whatever
zpool-implementation-specific shrinking method. If zpool_evictable()
is true for this zpool, then zpool_shrink() *might* callback to
zpool->ops->evict(), although it doesn't have to if it can shrink
without evictions. If zpool_evictable() is false, then zpool_shrink()
will never callback to zpool->ops->evict().
There is really no need for zswap to call different functions based on
whether the pool is evictable or not...is there?
On Wed, Jan 10, 2018 at 03:06:47PM -0500, Dan Streetman wrote:
> On Tue, Jan 9, 2018 at 5:47 PM, Yu Zhao <[email protected]> wrote:
> > On Tue, Jan 09, 2018 at 01:25:18PM -0500, Dan Streetman wrote:
> >> On Mon, Jan 8, 2018 at 5:51 PM, Yu Zhao <[email protected]> wrote:
> >> > We waste sizeof(swp_entry_t) for zswap header when using zsmalloc
> >> > as zpool driver because zsmalloc doesn't support eviction.
> >> >
> >> > Add zpool_shrinkable() to detect if zpool is shrinkable, and use
> >> > it in zswap to avoid waste memory for zswap header.
> >> >
> >> > Signed-off-by: Yu Zhao <[email protected]>
> >> > ---
> >> > include/linux/zpool.h | 2 ++
> >> > mm/zpool.c | 17 ++++++++++++++++-
> >> > mm/zsmalloc.c | 7 -------
> >> > mm/zswap.c | 20 ++++++++++----------
> >> > 4 files changed, 28 insertions(+), 18 deletions(-)
> >> >
> >> > diff --git a/include/linux/zpool.h b/include/linux/zpool.h
> >> > index 004ba807df96..3f0ac2ab74aa 100644
> >> > --- a/include/linux/zpool.h
> >> > +++ b/include/linux/zpool.h
> >> > @@ -108,4 +108,6 @@ void zpool_register_driver(struct zpool_driver *driver);
> >> >
> >> > int zpool_unregister_driver(struct zpool_driver *driver);
> >> >
> >> > +bool zpool_shrinkable(struct zpool *pool);
> >> > +
> >> > #endif
> >> > diff --git a/mm/zpool.c b/mm/zpool.c
> >> > index fd3ff719c32c..839d4234c540 100644
> >> > --- a/mm/zpool.c
> >> > +++ b/mm/zpool.c
> >> > @@ -296,7 +296,8 @@ void zpool_free(struct zpool *zpool, unsigned long handle)
> >> > int zpool_shrink(struct zpool *zpool, unsigned int pages,
> >> > unsigned int *reclaimed)
> >> > {
> >> > - return zpool->driver->shrink(zpool->pool, pages, reclaimed);
> >> > + return zpool_shrinkable(zpool) ?
> >> > + zpool->driver->shrink(zpool->pool, pages, reclaimed) : -EINVAL;
> >> > }
> >> >
> >> > /**
> >> > @@ -355,6 +356,20 @@ u64 zpool_get_total_size(struct zpool *zpool)
> >> > return zpool->driver->total_size(zpool->pool);
> >> > }
> >> >
> >> > +/**
> >> > + * zpool_shrinkable() - Test if zpool is shrinkable
> >> > + * @pool The zpool to test
> >> > + *
> >> > + * Zpool is only shrinkable when it's created with struct
> >> > + * zpool_ops.evict and its driver implements struct zpool_driver.shrink.
> >> > + *
> >> > + * Returns: true if shrinkable; false otherwise.
> >> > + */
> >> > +bool zpool_shrinkable(struct zpool *zpool)
> >> > +{
> >> > + return zpool->ops && zpool->ops->evict && zpool->driver->shrink;
> >>
> >> as these things won't ever change for the life of the zpool, it would
> >> probably be better to just check them at zpool creation time and set a
> >> single new zpool param, like 'zpool->shrinkable'. since this function
> >> will be called for every page that's swapped in or out, that may save
> >> a bit of time.
> >
> > Ack.
> >
> >> also re: calling it 'shrinkable' or 'evictable', the real thing zswap
> >> is interested in is if it needs to include the header info that
> >> zswap_writeback_entry (i.e. ops->evict) later needs, so yeah it does
> >> make more sense to call it zpool_evictable() and zpool->evictable.
> >> However, I think the function should still be zpool_shrink() and
> >> zpool->driver->shrink(), because it should be possible for
> >> zs_pool_shrink() to call the normal zsmalloc shrinker, instead of
> >> doing the zswap-style eviction, even if it doesn't do that currently.
> >
> > I agree we keep zpool_shrink(). It could either shrink pool if driver
> > supports slab shrinker by providing zpool->driver->shrink or evict
> > pages from pool if driver supports zpool->driver->evict (which in turn
> > calls ops->evict provided by zswap) or both.
> >
> > We can't use a single zpool->driver->callback to achieve both because
> > there will be no way for zswap to know if driver uses ops->evict thus
> > no way to determine if zswap_header is needed.
> >
> > So for now, I think it'd be better if we deleted zpool->driver->shrink
> > from zsmalloc and renamed it to zpool->driver->evict in zbud. Later
> > if we decide zpool_shrink should also call zsmalloc slab shrinker, we
> > add a new callback.
>
> Well, I think shrink vs evict an implementation detail, isn't it?
> That is, from zswap's perspective, there should be:
>
> zpool_evictable()
> if true, zswap needs to include the header on each compressed page,
> because the zpool may callback zpool->ops->evict() which calls
> zswap_writeback_entry() which expects the entry to start with a zswap
> header.
> if false, zswap doesn't need to include the header, because the zpool
> will never, ever call zpool->ops->evict
>
> zpool_shrink()
> this will try to shrink the zpool, using whatever
> zpool-implementation-specific shrinking method. If zpool_evictable()
> is true for this zpool, then zpool_shrink() *might* callback to
> zpool->ops->evict(), although it doesn't have to if it can shrink
> without evictions. If zpool_evictable() is false, then zpool_shrink()
> will never callback to zpool->ops->evict().
>
> There is really no need for zswap to call different functions based on
> whether the pool is evictable or not...is there?
Thanks. I'd prefer if driver drew a clear line between its defrag and
eviction capabilities. But I have no objection to leaving them as its
internal implementation details and keeping current name of
zpool->driver->shrink.
We waste sizeof(swp_entry_t) for zswap header when using zsmalloc
as zpool driver because zsmalloc doesn't support eviction.
Add zpool_evictable() to detect if zpool is potentially evictable,
and use it in zswap to avoid waste memory for zswap header.
Signed-off-by: Yu Zhao <[email protected]>
---
include/linux/zpool.h | 2 ++
mm/zpool.c | 26 ++++++++++++++++++++++++--
mm/zsmalloc.c | 7 -------
mm/zswap.c | 20 ++++++++++----------
4 files changed, 36 insertions(+), 19 deletions(-)
diff --git a/include/linux/zpool.h b/include/linux/zpool.h
index 004ba807df96..7238865e75b0 100644
--- a/include/linux/zpool.h
+++ b/include/linux/zpool.h
@@ -108,4 +108,6 @@ void zpool_register_driver(struct zpool_driver *driver);
int zpool_unregister_driver(struct zpool_driver *driver);
+bool zpool_evictable(struct zpool *pool);
+
#endif
diff --git a/mm/zpool.c b/mm/zpool.c
index fd3ff719c32c..ec63ef32d73d 100644
--- a/mm/zpool.c
+++ b/mm/zpool.c
@@ -21,6 +21,7 @@ struct zpool {
struct zpool_driver *driver;
void *pool;
const struct zpool_ops *ops;
+ bool evictable;
struct list_head list;
};
@@ -142,7 +143,7 @@ EXPORT_SYMBOL(zpool_has_pool);
*
* This creates a new zpool of the specified type. The gfp flags will be
* used when allocating memory, if the implementation supports it. If the
- * ops param is NULL, then the created zpool will not be shrinkable.
+ * ops param is NULL, then the created zpool will not be evictable.
*
* Implementations must guarantee this to be thread-safe.
*
@@ -180,6 +181,8 @@ struct zpool *zpool_create_pool(const char *type, const char *name, gfp_t gfp,
zpool->driver = driver;
zpool->pool = driver->create(name, gfp, ops, zpool);
zpool->ops = ops;
+ zpool->evictable = zpool->driver->shrink &&
+ zpool->ops && zpool->ops->evict;
if (!zpool->pool) {
pr_err("couldn't create %s pool\n", type);
@@ -296,7 +299,8 @@ void zpool_free(struct zpool *zpool, unsigned long handle)
int zpool_shrink(struct zpool *zpool, unsigned int pages,
unsigned int *reclaimed)
{
- return zpool->driver->shrink(zpool->pool, pages, reclaimed);
+ return zpool->driver->shrink ?
+ zpool->driver->shrink(zpool->pool, pages, reclaimed) : -EINVAL;
}
/**
@@ -355,6 +359,24 @@ u64 zpool_get_total_size(struct zpool *zpool)
return zpool->driver->total_size(zpool->pool);
}
+/**
+ * zpool_evictable() - Test if zpool is potentially evictable
+ * @pool The zpool to test
+ *
+ * Zpool is only potentially evictable when it's created with struct
+ * zpool_ops.evict and its driver implements struct zpool_driver.shrink.
+ *
+ * However, it doesn't necessarily mean driver will use zpool_ops.evict
+ * in its implementation of zpool_driver.shrink. It could do internal
+ * defragmentation instead.
+ *
+ * Returns: true if potentially evictable; false otherwise.
+ */
+bool zpool_evictable(struct zpool *zpool)
+{
+ return zpool->evictable;
+}
+
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Dan Streetman <[email protected]>");
MODULE_DESCRIPTION("Common API for compressed memory storage");
diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 683c0651098c..9cc741bcdb32 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -407,12 +407,6 @@ static void zs_zpool_free(void *pool, unsigned long handle)
zs_free(pool, handle);
}
-static int zs_zpool_shrink(void *pool, unsigned int pages,
- unsigned int *reclaimed)
-{
- return -EINVAL;
-}
-
static void *zs_zpool_map(void *pool, unsigned long handle,
enum zpool_mapmode mm)
{
@@ -450,7 +444,6 @@ static struct zpool_driver zs_zpool_driver = {
.destroy = zs_zpool_destroy,
.malloc = zs_zpool_malloc,
.free = zs_zpool_free,
- .shrink = zs_zpool_shrink,
.map = zs_zpool_map,
.unmap = zs_zpool_unmap,
.total_size = zs_zpool_total_size,
diff --git a/mm/zswap.c b/mm/zswap.c
index d39581a076c3..52a9ab519ab2 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -964,11 +964,11 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
struct zswap_entry *entry, *dupentry;
struct crypto_comp *tfm;
int ret;
- unsigned int dlen = PAGE_SIZE, len;
+ unsigned int hlen, dlen = PAGE_SIZE;
unsigned long handle;
char *buf;
u8 *src, *dst;
- struct zswap_header *zhdr;
+ struct zswap_header zhdr = { .swpentry = swp_entry(type, offset) };
if (!zswap_enabled || !tree) {
ret = -ENODEV;
@@ -1013,8 +1013,8 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
}
/* store */
- len = dlen + sizeof(struct zswap_header);
- ret = zpool_malloc(entry->pool->zpool, len,
+ hlen = zpool_evictable(entry->pool->zpool) ? sizeof(zhdr) : 0;
+ ret = zpool_malloc(entry->pool->zpool, hlen + dlen,
__GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM,
&handle);
if (ret == -ENOSPC) {
@@ -1025,10 +1025,9 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
zswap_reject_alloc_fail++;
goto put_dstmem;
}
- zhdr = zpool_map_handle(entry->pool->zpool, handle, ZPOOL_MM_RW);
- zhdr->swpentry = swp_entry(type, offset);
- buf = (u8 *)(zhdr + 1);
- memcpy(buf, dst, dlen);
+ buf = zpool_map_handle(entry->pool->zpool, handle, ZPOOL_MM_RW);
+ memcpy(buf, &zhdr, hlen);
+ memcpy(buf + hlen, dst, dlen);
zpool_unmap_handle(entry->pool->zpool, handle);
put_cpu_var(zswap_dstmem);
@@ -1091,8 +1090,9 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset,
/* decompress */
dlen = PAGE_SIZE;
- src = (u8 *)zpool_map_handle(entry->pool->zpool, entry->handle,
- ZPOOL_MM_RO) + sizeof(struct zswap_header);
+ src = zpool_map_handle(entry->pool->zpool, entry->handle, ZPOOL_MM_RO);
+ if (zpool_evictable(entry->pool->zpool))
+ src += sizeof(struct zswap_header);
dst = kmap_atomic(page);
tfm = *get_cpu_ptr(entry->pool->tfm);
ret = crypto_comp_decompress(tfm, src, entry->length, dst, &dlen);
--
2.16.0.rc1.238.g530d649a79-goog
We waste sizeof(swp_entry_t) for zswap header when using zsmalloc
as zpool driver because zsmalloc doesn't support eviction.
Add zpool_evictable() to detect if zpool is potentially evictable,
and use it in zswap to avoid waste memory for zswap header.
Signed-off-by: Yu Zhao <[email protected]>
---
include/linux/zpool.h | 2 ++
mm/zpool.c | 25 +++++++++++++++++++++++--
mm/zsmalloc.c | 7 -------
mm/zswap.c | 20 ++++++++++----------
4 files changed, 35 insertions(+), 19 deletions(-)
diff --git a/include/linux/zpool.h b/include/linux/zpool.h
index 004ba807df96..7238865e75b0 100644
--- a/include/linux/zpool.h
+++ b/include/linux/zpool.h
@@ -108,4 +108,6 @@ void zpool_register_driver(struct zpool_driver *driver);
int zpool_unregister_driver(struct zpool_driver *driver);
+bool zpool_evictable(struct zpool *pool);
+
#endif
diff --git a/mm/zpool.c b/mm/zpool.c
index fd3ff719c32c..e1e7aa6d1d06 100644
--- a/mm/zpool.c
+++ b/mm/zpool.c
@@ -21,6 +21,7 @@ struct zpool {
struct zpool_driver *driver;
void *pool;
const struct zpool_ops *ops;
+ bool evictable;
struct list_head list;
};
@@ -142,7 +143,7 @@ EXPORT_SYMBOL(zpool_has_pool);
*
* This creates a new zpool of the specified type. The gfp flags will be
* used when allocating memory, if the implementation supports it. If the
- * ops param is NULL, then the created zpool will not be shrinkable.
+ * ops param is NULL, then the created zpool will not be evictable.
*
* Implementations must guarantee this to be thread-safe.
*
@@ -180,6 +181,7 @@ struct zpool *zpool_create_pool(const char *type, const char *name, gfp_t gfp,
zpool->driver = driver;
zpool->pool = driver->create(name, gfp, ops, zpool);
zpool->ops = ops;
+ zpool->evictable = driver->shrink && ops && ops->evict;
if (!zpool->pool) {
pr_err("couldn't create %s pool\n", type);
@@ -296,7 +298,8 @@ void zpool_free(struct zpool *zpool, unsigned long handle)
int zpool_shrink(struct zpool *zpool, unsigned int pages,
unsigned int *reclaimed)
{
- return zpool->driver->shrink(zpool->pool, pages, reclaimed);
+ return zpool->driver->shrink ?
+ zpool->driver->shrink(zpool->pool, pages, reclaimed) : -EINVAL;
}
/**
@@ -355,6 +358,24 @@ u64 zpool_get_total_size(struct zpool *zpool)
return zpool->driver->total_size(zpool->pool);
}
+/**
+ * zpool_evictable() - Test if zpool is potentially evictable
+ * @pool The zpool to test
+ *
+ * Zpool is only potentially evictable when it's created with struct
+ * zpool_ops.evict and its driver implements struct zpool_driver.shrink.
+ *
+ * However, it doesn't necessarily mean driver will use zpool_ops.evict
+ * in its implementation of zpool_driver.shrink. It could do internal
+ * defragmentation instead.
+ *
+ * Returns: true if potentially evictable; false otherwise.
+ */
+bool zpool_evictable(struct zpool *zpool)
+{
+ return zpool->evictable;
+}
+
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Dan Streetman <[email protected]>");
MODULE_DESCRIPTION("Common API for compressed memory storage");
diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 683c0651098c..9cc741bcdb32 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -407,12 +407,6 @@ static void zs_zpool_free(void *pool, unsigned long handle)
zs_free(pool, handle);
}
-static int zs_zpool_shrink(void *pool, unsigned int pages,
- unsigned int *reclaimed)
-{
- return -EINVAL;
-}
-
static void *zs_zpool_map(void *pool, unsigned long handle,
enum zpool_mapmode mm)
{
@@ -450,7 +444,6 @@ static struct zpool_driver zs_zpool_driver = {
.destroy = zs_zpool_destroy,
.malloc = zs_zpool_malloc,
.free = zs_zpool_free,
- .shrink = zs_zpool_shrink,
.map = zs_zpool_map,
.unmap = zs_zpool_unmap,
.total_size = zs_zpool_total_size,
diff --git a/mm/zswap.c b/mm/zswap.c
index d39581a076c3..52a9ab519ab2 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -964,11 +964,11 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
struct zswap_entry *entry, *dupentry;
struct crypto_comp *tfm;
int ret;
- unsigned int dlen = PAGE_SIZE, len;
+ unsigned int hlen, dlen = PAGE_SIZE;
unsigned long handle;
char *buf;
u8 *src, *dst;
- struct zswap_header *zhdr;
+ struct zswap_header zhdr = { .swpentry = swp_entry(type, offset) };
if (!zswap_enabled || !tree) {
ret = -ENODEV;
@@ -1013,8 +1013,8 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
}
/* store */
- len = dlen + sizeof(struct zswap_header);
- ret = zpool_malloc(entry->pool->zpool, len,
+ hlen = zpool_evictable(entry->pool->zpool) ? sizeof(zhdr) : 0;
+ ret = zpool_malloc(entry->pool->zpool, hlen + dlen,
__GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM,
&handle);
if (ret == -ENOSPC) {
@@ -1025,10 +1025,9 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
zswap_reject_alloc_fail++;
goto put_dstmem;
}
- zhdr = zpool_map_handle(entry->pool->zpool, handle, ZPOOL_MM_RW);
- zhdr->swpentry = swp_entry(type, offset);
- buf = (u8 *)(zhdr + 1);
- memcpy(buf, dst, dlen);
+ buf = zpool_map_handle(entry->pool->zpool, handle, ZPOOL_MM_RW);
+ memcpy(buf, &zhdr, hlen);
+ memcpy(buf + hlen, dst, dlen);
zpool_unmap_handle(entry->pool->zpool, handle);
put_cpu_var(zswap_dstmem);
@@ -1091,8 +1090,9 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset,
/* decompress */
dlen = PAGE_SIZE;
- src = (u8 *)zpool_map_handle(entry->pool->zpool, entry->handle,
- ZPOOL_MM_RO) + sizeof(struct zswap_header);
+ src = zpool_map_handle(entry->pool->zpool, entry->handle, ZPOOL_MM_RO);
+ if (zpool_evictable(entry->pool->zpool))
+ src += sizeof(struct zswap_header);
dst = kmap_atomic(page);
tfm = *get_cpu_ptr(entry->pool->tfm);
ret = crypto_comp_decompress(tfm, src, entry->length, dst, &dlen);
--
2.16.0.rc1.238.g530d649a79-goog
On Wed, Jan 10, 2018 at 02:47:41PM -0800, Yu Zhao wrote:
> We waste sizeof(swp_entry_t) for zswap header when using zsmalloc
> as zpool driver because zsmalloc doesn't support eviction.
>
> Add zpool_evictable() to detect if zpool is potentially evictable,
> and use it in zswap to avoid waste memory for zswap header.
>
> Signed-off-by: Yu Zhao <[email protected]>
> ---
> include/linux/zpool.h | 2 ++
> mm/zpool.c | 26 ++++++++++++++++++++++++--
> mm/zsmalloc.c | 7 -------
> mm/zswap.c | 20 ++++++++++----------
> 4 files changed, 36 insertions(+), 19 deletions(-)
>
> diff --git a/include/linux/zpool.h b/include/linux/zpool.h
> index 004ba807df96..7238865e75b0 100644
> --- a/include/linux/zpool.h
> +++ b/include/linux/zpool.h
> @@ -108,4 +108,6 @@ void zpool_register_driver(struct zpool_driver *driver);
>
> int zpool_unregister_driver(struct zpool_driver *driver);
>
> +bool zpool_evictable(struct zpool *pool);
> +
> #endif
> diff --git a/mm/zpool.c b/mm/zpool.c
> index fd3ff719c32c..ec63ef32d73d 100644
> --- a/mm/zpool.c
> +++ b/mm/zpool.c
> @@ -21,6 +21,7 @@ struct zpool {
> struct zpool_driver *driver;
> void *pool;
> const struct zpool_ops *ops;
> + bool evictable;
>
> struct list_head list;
> };
> @@ -142,7 +143,7 @@ EXPORT_SYMBOL(zpool_has_pool);
> *
> * This creates a new zpool of the specified type. The gfp flags will be
> * used when allocating memory, if the implementation supports it. If the
> - * ops param is NULL, then the created zpool will not be shrinkable.
> + * ops param is NULL, then the created zpool will not be evictable.
> *
> * Implementations must guarantee this to be thread-safe.
> *
> @@ -180,6 +181,8 @@ struct zpool *zpool_create_pool(const char *type, const char *name, gfp_t gfp,
> zpool->driver = driver;
> zpool->pool = driver->create(name, gfp, ops, zpool);
> zpool->ops = ops;
> + zpool->evictable = zpool->driver->shrink &&
> + zpool->ops && zpool->ops->evict;
The zpool->" prefix is a result out copy & paste. Fixed in the next
version. Sorry for the spam.
Hello,
Yu Zhao, Dan, sorry for the delay
On (01/10/18 15:06), Dan Streetman wrote:
[..]
> Well, I think shrink vs evict an implementation detail, isn't it?
> That is, from zswap's perspective, there should be:
>
> zpool_evictable()
> if true, zswap needs to include the header on each compressed page,
> because the zpool may callback zpool->ops->evict() which calls
> zswap_writeback_entry() which expects the entry to start with a zswap
> header.
> if false, zswap doesn't need to include the header, because the zpool
> will never, ever call zpool->ops->evict
>
> zpool_shrink()
> this will try to shrink the zpool, using whatever
> zpool-implementation-specific shrinking method. If zpool_evictable()
> is true for this zpool, then zpool_shrink() *might* callback to
> zpool->ops->evict(), although it doesn't have to if it can shrink
> without evictions. If zpool_evictable() is false, then zpool_shrink()
> will never callback to zpool->ops->evict().
ACK on this!
-ss
On (01/10/18 14:56), Yu Zhao wrote:
[..]
> We waste sizeof(swp_entry_t) for zswap header when using zsmalloc
> as zpool driver because zsmalloc doesn't support eviction.
>
> Add zpool_evictable() to detect if zpool is potentially evictable,
> and use it in zswap to avoid waste memory for zswap header.
>
> Signed-off-by: Yu Zhao <[email protected]>
looks good to me.
FWIW,
Reviewed-by: Sergey Senozhatsky <[email protected]>
-ss
On Wed, Jan 10, 2018 at 5:56 PM, Yu Zhao <[email protected]> wrote:
> We waste sizeof(swp_entry_t) for zswap header when using zsmalloc
> as zpool driver because zsmalloc doesn't support eviction.
>
> Add zpool_evictable() to detect if zpool is potentially evictable,
> and use it in zswap to avoid waste memory for zswap header.
>
> Signed-off-by: Yu Zhao <[email protected]>
> ---
> include/linux/zpool.h | 2 ++
> mm/zpool.c | 25 +++++++++++++++++++++++--
> mm/zsmalloc.c | 7 -------
> mm/zswap.c | 20 ++++++++++----------
> 4 files changed, 35 insertions(+), 19 deletions(-)
>
> diff --git a/include/linux/zpool.h b/include/linux/zpool.h
> index 004ba807df96..7238865e75b0 100644
> --- a/include/linux/zpool.h
> +++ b/include/linux/zpool.h
> @@ -108,4 +108,6 @@ void zpool_register_driver(struct zpool_driver *driver);
>
> int zpool_unregister_driver(struct zpool_driver *driver);
>
> +bool zpool_evictable(struct zpool *pool);
> +
> #endif
> diff --git a/mm/zpool.c b/mm/zpool.c
> index fd3ff719c32c..e1e7aa6d1d06 100644
> --- a/mm/zpool.c
> +++ b/mm/zpool.c
> @@ -21,6 +21,7 @@ struct zpool {
> struct zpool_driver *driver;
> void *pool;
> const struct zpool_ops *ops;
> + bool evictable;
>
> struct list_head list;
> };
> @@ -142,7 +143,7 @@ EXPORT_SYMBOL(zpool_has_pool);
> *
> * This creates a new zpool of the specified type. The gfp flags will be
> * used when allocating memory, if the implementation supports it. If the
> - * ops param is NULL, then the created zpool will not be shrinkable.
> + * ops param is NULL, then the created zpool will not be evictable.
> *
> * Implementations must guarantee this to be thread-safe.
> *
> @@ -180,6 +181,7 @@ struct zpool *zpool_create_pool(const char *type, const char *name, gfp_t gfp,
> zpool->driver = driver;
> zpool->pool = driver->create(name, gfp, ops, zpool);
> zpool->ops = ops;
> + zpool->evictable = driver->shrink && ops && ops->evict;
Since the ops->evict comes from zswap (and is never omitted), if we do
restore zs_zpool_shrink() in the future to call the zsmalloc shrinker
(and not do eviction), we'll have to add a driver->evictable bool to
check here as well.
But for now this is good, the zpools with driver->shrink do eviction,
zsmalloc doesn't do eviction and won't have driver->shrink.
Acked-by: Dan Streetman <[email protected]>
Thanks!
>
> if (!zpool->pool) {
> pr_err("couldn't create %s pool\n", type);
> @@ -296,7 +298,8 @@ void zpool_free(struct zpool *zpool, unsigned long handle)
> int zpool_shrink(struct zpool *zpool, unsigned int pages,
> unsigned int *reclaimed)
> {
> - return zpool->driver->shrink(zpool->pool, pages, reclaimed);
> + return zpool->driver->shrink ?
> + zpool->driver->shrink(zpool->pool, pages, reclaimed) : -EINVAL;
> }
>
> /**
> @@ -355,6 +358,24 @@ u64 zpool_get_total_size(struct zpool *zpool)
> return zpool->driver->total_size(zpool->pool);
> }
>
> +/**
> + * zpool_evictable() - Test if zpool is potentially evictable
> + * @pool The zpool to test
> + *
> + * Zpool is only potentially evictable when it's created with struct
> + * zpool_ops.evict and its driver implements struct zpool_driver.shrink.
> + *
> + * However, it doesn't necessarily mean driver will use zpool_ops.evict
> + * in its implementation of zpool_driver.shrink. It could do internal
> + * defragmentation instead.
> + *
> + * Returns: true if potentially evictable; false otherwise.
> + */
> +bool zpool_evictable(struct zpool *zpool)
> +{
> + return zpool->evictable;
> +}
> +
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("Dan Streetman <[email protected]>");
> MODULE_DESCRIPTION("Common API for compressed memory storage");
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index 683c0651098c..9cc741bcdb32 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -407,12 +407,6 @@ static void zs_zpool_free(void *pool, unsigned long handle)
> zs_free(pool, handle);
> }
>
> -static int zs_zpool_shrink(void *pool, unsigned int pages,
> - unsigned int *reclaimed)
> -{
> - return -EINVAL;
> -}
> -
> static void *zs_zpool_map(void *pool, unsigned long handle,
> enum zpool_mapmode mm)
> {
> @@ -450,7 +444,6 @@ static struct zpool_driver zs_zpool_driver = {
> .destroy = zs_zpool_destroy,
> .malloc = zs_zpool_malloc,
> .free = zs_zpool_free,
> - .shrink = zs_zpool_shrink,
> .map = zs_zpool_map,
> .unmap = zs_zpool_unmap,
> .total_size = zs_zpool_total_size,
> diff --git a/mm/zswap.c b/mm/zswap.c
> index d39581a076c3..52a9ab519ab2 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -964,11 +964,11 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
> struct zswap_entry *entry, *dupentry;
> struct crypto_comp *tfm;
> int ret;
> - unsigned int dlen = PAGE_SIZE, len;
> + unsigned int hlen, dlen = PAGE_SIZE;
> unsigned long handle;
> char *buf;
> u8 *src, *dst;
> - struct zswap_header *zhdr;
> + struct zswap_header zhdr = { .swpentry = swp_entry(type, offset) };
>
> if (!zswap_enabled || !tree) {
> ret = -ENODEV;
> @@ -1013,8 +1013,8 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
> }
>
> /* store */
> - len = dlen + sizeof(struct zswap_header);
> - ret = zpool_malloc(entry->pool->zpool, len,
> + hlen = zpool_evictable(entry->pool->zpool) ? sizeof(zhdr) : 0;
> + ret = zpool_malloc(entry->pool->zpool, hlen + dlen,
> __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM,
> &handle);
> if (ret == -ENOSPC) {
> @@ -1025,10 +1025,9 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
> zswap_reject_alloc_fail++;
> goto put_dstmem;
> }
> - zhdr = zpool_map_handle(entry->pool->zpool, handle, ZPOOL_MM_RW);
> - zhdr->swpentry = swp_entry(type, offset);
> - buf = (u8 *)(zhdr + 1);
> - memcpy(buf, dst, dlen);
> + buf = zpool_map_handle(entry->pool->zpool, handle, ZPOOL_MM_RW);
> + memcpy(buf, &zhdr, hlen);
> + memcpy(buf + hlen, dst, dlen);
> zpool_unmap_handle(entry->pool->zpool, handle);
> put_cpu_var(zswap_dstmem);
>
> @@ -1091,8 +1090,9 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset,
>
> /* decompress */
> dlen = PAGE_SIZE;
> - src = (u8 *)zpool_map_handle(entry->pool->zpool, entry->handle,
> - ZPOOL_MM_RO) + sizeof(struct zswap_header);
> + src = zpool_map_handle(entry->pool->zpool, entry->handle, ZPOOL_MM_RO);
> + if (zpool_evictable(entry->pool->zpool))
> + src += sizeof(struct zswap_header);
> dst = kmap_atomic(page);
> tfm = *get_cpu_ptr(entry->pool->tfm);
> ret = crypto_comp_decompress(tfm, src, entry->length, dst, &dlen);
> --
> 2.16.0.rc1.238.g530d649a79-goog
>