Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Minchan Kim]

On Wed, Nov 06, 2013 at 07:05:11PM -0800, Greg KH wrote:
> On Wed, Nov 06, 2013 at 03:46:19PM -0800, Nitin Gupta wrote:
>  > I'm getting really tired of them hanging around in here for many years
> > > now...
> > >
> > 
> > Minchan has tried many times to promote zram out of staging. This was
> > his most recent attempt:
> > 
> > https://lkml.org/lkml/2013/8/21/54
> > 
> > There he provided arguments for zram inclusion, how it can help in
> > situations where zswap can't and why generalizing /dev/ramX would
> > not be a great idea. So, cannot say why it wasn't picked up
> > for inclusion at that time.
> > 
> > > Should I just remove them if no one is working on getting them merged
> > > "properly"?
> > >
> > 
> > Please refer the mail thread (link above) and see Minchan's
> > justifications for zram.
> > If they don't sound convincing enough then please remove zram+zsmalloc
> > from staging.
> 
> You don't need to be convincing me, you need to be convincing the
> maintainers of the area of the kernel you are working with.
> 
> And since the last time you all tried to get this merged was back in
> August, I'm feeling that you all have given up, so it needs to be
> deleted.  I'll go do that for 3.14, and if someone wants to pick it up
> and merge it properly, they can easily revert it.

I'm guilty and I have been busy by other stuff. Sorry for that.
Fortunately, I discussed this issue with Hugh in this Linuxcon for a
long time(Thanks Hugh!) he felt zram's block device abstraction is
better design rather than frontswap backend stuff although it's a question
where we put zsmalloc. I will CC Hugh because many of things is related
to swap subsystem and his opinion is really important.
And I discussed it with Rik and he feel positive about zram.

Last impression Andrw gave me by private mail is he want to merge
zram's functionality into zswap or vise versa.
If I misunderstood, please correct me.
I understand his concern but I guess he didn't have a time to read
my long description due to a ton of works at that time.
So, I will try one more time.
I hope I'd like to listen feedback than *silence* so that we can
move forward than stall.

Recently, Bob tried to move zsmalloc under mm directory to unify
zram and zswap with adding pseudo block device in zswap(It's
very weired to me. I think it's horrible monster which is lying
between mm and block in layering POV) but he was ignoring zram's
block device (a.k.a zram-blk) feature and considered only swap
usecase of zram, in turn, it lose zram's good concept. 
I already convered other topics Bob raised in this thread[1]
and why I think zram is better in the thread.

Will repeat one more time and hope gray beards penguins grab a
time in this time and they give a conclusion/direction to me so
that we don't lose lots of user and functionality.

========== &< ===========

Mel raised an another issue in v6, "maintainance headache".
He claimed zswap and zram has a similar goal that is to compresss
swap pages so if we promote zram, maintainance headache happens
sometime by diverging implementaion between zswap and zram
so that he want to unify zram and zswap. For it, he want zswap
to implement pseudo block device like Bob did to emulate zram so
zswap can have an advantage of writeback as well as zram's benefit.
But I wonder frontswap-based zswap's writeback is really good
approach for writeback POV. I think that problem isn't only
specific for zswap. If we want to configure multiple swap hierarchy
with various speed device such as RAM, NVRAM, SSD, eMMC, NAS etc,
it would be a general problem. So we should think of more general
approach. At a glance, I can see two approach.

First, VM could be aware of heterogeneous swap configuration
so it could aim for being able to configure cache hierarchy
among swap devices. It may need indirction layer on swap, which
was already talked about that way so VM can migrate a block from
A to B easily. It will support various configuration with VM's
hints, maybe, in future.
http://lkml.indiana.edu/hypermail/linux/kernel/1203.3/03812.html

Second, as more practical solution, we could use device mapper like
dm-cache(https://lwn.net/Articles/540996/), which makes it very
flexible. Now, it supports various configruation and cache policy
(block size, writeback/writethrough, LRU, MFU although MQ is merged
now) so it would be good fit for our purpose. Even, it can make zram
support writeback. I tested it following as following scenario
in KVM 4 CPU, 1G DRAM with background 800M memory hogger, which is
allocates random data up to 800M.

1) zram swap disk 1G, untar kernel.tgz to tmpfs, build -j 4
   Fail to untar due to shortage of memory space by tmpfs default size limit

2) zram swap disk 1G, untar kernel.tgz to ext2 on zram-blk, build -j 4
   OOM happens while building the kernel but it untar successfully
   on ext2 based on zram-blk. The reason OOM happend is zram can not find
   free pages from main memory to store swap out pages although empty
   swap space is still enough.

3) dm-cache swap disk 1G, untar kernel.tgz to ext2 on zram-blk, build -j 4
   dmcache consists of zram-meta 10M, zram-cache 1G and real swap storage 1G
   No OOM happens and successfully building done.

Above tests proves zram can support writeback into real swap storage
so that zram-cache can always have a free space. If necessary, we could
add new plugin in dm-cache. I see It's really flexible and well-layered
architecure so zram-blk's concept is good for us and it has lots of
potential to be enhanced by MM/FS/Block developers.

As other disadvantage of zswap writeback, frontswap's semantic is
synchronous API so zswap should decompress in memory zpage
right before writeback and even, it writes pages one by one,
not a batch. If we extend frontswap API, we would enhance it but
I belive we can do better in device mapper layer which is aware of
block align, bandwidth, mapping table, asynchronous and lots of hints
from the block layer. Nonetheless, if we should merge zram's
functionality to zswap, I think zram should include zswap's
functionaliy(But I hope it will never happen) because old age zram
already has lots of real users rather than new young zswap so it's
more handy to unify them with keeping changelog which is one of
valuable things getting from staging stay for a long time.

The reason zram doesn't support writeback until now is just shortage
of needs. The zram's main customers were embedded people so writeback
into real swap storage is too bad for interactivity and wear-leveling
on low falsh devices. But like above, zram has a potential to support
writeback with other block drivers or more reasonable VM enhance
so I'd like to claim zram's block concept is really good.

Another zram-blk's usecase is following as.
The admin can format /dev/zramX with any FS and mount on it.
It could help small memory system, too. For exmaple, many embedded
system don't have swap so although tmpfs can support swapout,
it's pointless. Then, let's assume temp file growing up until half
of system memory once in a while. We don't want to write it on flash
by wear-leveing issue and response problem so we want to keep in-memory.
But if we use tmpfs, it should evict half of working set to cover them
when the size reach peak. In the case, zram-blk would be good fit, too.

I'd like to enhance zram with more features like zsmalloc-compaction,
, async I/O, parallel decompression and so on but zram developers cannot
do it now because Greg, staging maintainer, doesn't want to add new feature
until promotion is done because zram have been in staging for a very long time.
Acutally, some patches about enhance are pending for a long time.

[1] https://lkml.org/lkml/2013/8/21/141

> 
> thanks,
> 
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Kind regards,
Minchan Kim

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Luigi Semenzato]

If I may add my usual 2c (and some news):

zram is used by default on all Chrome OS devices.  I can't say how
many devices, but it's not a small number, google it, and it's an
important market, low-end laptops for education and the less affluent.
 It has been available experimentally for well over a year.

Android 4.4 KitKat is also using zram, to better support devices with
less than 1 MB RAM.  (That's the news.)

When comparing the relative advantages of the two subsystems (zram and
zswap), let's not forget that considerable effort goes into in tuning
and bug fixing for specific use cases---possibly even more than the
initial development effort.  Zram has not just been sitting around in
drivers/staging, it's in serious use.

If we were to judge systems based merely on theoretical technical
merit, then we should consider switching en masse to FreeBSD.  (I said
we should *consider* :).

I am very familiar with the limitations of zram, but it works well and
I think it would be wise to keep supporting it.  Besides, it's small
and AFAICT it interfaces cleanly with the rest of the system, so I
don't see what the big deal is.

Thanks!



On Wed, Nov 6, 2013 at 11:04 PM, Minchan Kim <minchan@kernel.org> wrote:
> On Wed, Nov 06, 2013 at 07:05:11PM -0800, Greg KH wrote:
>> On Wed, Nov 06, 2013 at 03:46:19PM -0800, Nitin Gupta wrote:
>>  > I'm getting really tired of them hanging around in here for many years
>> > > now...
>> > >
>> >
>> > Minchan has tried many times to promote zram out of staging. This was
>> > his most recent attempt:
>> >
>> > https://lkml.org/lkml/2013/8/21/54
>> >
>> > There he provided arguments for zram inclusion, how it can help in
>> > situations where zswap can't and why generalizing /dev/ramX would
>> > not be a great idea. So, cannot say why it wasn't picked up
>> > for inclusion at that time.
>> >
>> > > Should I just remove them if no one is working on getting them merged
>> > > "properly"?
>> > >
>> >
>> > Please refer the mail thread (link above) and see Minchan's
>> > justifications for zram.
>> > If they don't sound convincing enough then please remove zram+zsmalloc
>> > from staging.
>>
>> You don't need to be convincing me, you need to be convincing the
>> maintainers of the area of the kernel you are working with.
>>
>> And since the last time you all tried to get this merged was back in
>> August, I'm feeling that you all have given up, so it needs to be
>> deleted.  I'll go do that for 3.14, and if someone wants to pick it up
>> and merge it properly, they can easily revert it.
>
> I'm guilty and I have been busy by other stuff. Sorry for that.
> Fortunately, I discussed this issue with Hugh in this Linuxcon for a
> long time(Thanks Hugh!) he felt zram's block device abstraction is
> better design rather than frontswap backend stuff although it's a question
> where we put zsmalloc. I will CC Hugh because many of things is related
> to swap subsystem and his opinion is really important.
> And I discussed it with Rik and he feel positive about zram.
>
> Last impression Andrw gave me by private mail is he want to merge
> zram's functionality into zswap or vise versa.
> If I misunderstood, please correct me.
> I understand his concern but I guess he didn't have a time to read
> my long description due to a ton of works at that time.
> So, I will try one more time.
> I hope I'd like to listen feedback than *silence* so that we can
> move forward than stall.
>
> Recently, Bob tried to move zsmalloc under mm directory to unify
> zram and zswap with adding pseudo block device in zswap(It's
> very weired to me. I think it's horrible monster which is lying
> between mm and block in layering POV) but he was ignoring zram's
> block device (a.k.a zram-blk) feature and considered only swap
> usecase of zram, in turn, it lose zram's good concept.
> I already convered other topics Bob raised in this thread[1]
> and why I think zram is better in the thread.
>
> Will repeat one more time and hope gray beards penguins grab a
> time in this time and they give a conclusion/direction to me so
> that we don't lose lots of user and functionality.
>
> ========== &< ===========
>
> Mel raised an another issue in v6, "maintainance headache".
> He claimed zswap and zram has a similar goal that is to compresss
> swap pages so if we promote zram, maintainance headache happens
> sometime by diverging implementaion between zswap and zram
> so that he want to unify zram and zswap. For it, he want zswap
> to implement pseudo block device like Bob did to emulate zram so
> zswap can have an advantage of writeback as well as zram's benefit.
> But I wonder frontswap-based zswap's writeback is really good
> approach for writeback POV. I think that problem isn't only
> specific for zswap. If we want to configure multiple swap hierarchy
> with various speed device such as RAM, NVRAM, SSD, eMMC, NAS etc,
> it would be a general problem. So we should think of more general
> approach. At a glance, I can see two approach.
>
> First, VM could be aware of heterogeneous swap configuration
> so it could aim for being able to configure cache hierarchy
> among swap devices. It may need indirction layer on swap, which
> was already talked about that way so VM can migrate a block from
> A to B easily. It will support various configuration with VM's
> hints, maybe, in future.
> http://lkml.indiana.edu/hypermail/linux/kernel/1203.3/03812.html
>
> Second, as more practical solution, we could use device mapper like
> dm-cache(https://lwn.net/Articles/540996/), which makes it very
> flexible. Now, it supports various configruation and cache policy
> (block size, writeback/writethrough, LRU, MFU although MQ is merged
> now) so it would be good fit for our purpose. Even, it can make zram
> support writeback. I tested it following as following scenario
> in KVM 4 CPU, 1G DRAM with background 800M memory hogger, which is
> allocates random data up to 800M.
>
> 1) zram swap disk 1G, untar kernel.tgz to tmpfs, build -j 4
>    Fail to untar due to shortage of memory space by tmpfs default size limit
>
> 2) zram swap disk 1G, untar kernel.tgz to ext2 on zram-blk, build -j 4
>    OOM happens while building the kernel but it untar successfully
>    on ext2 based on zram-blk. The reason OOM happend is zram can not find
>    free pages from main memory to store swap out pages although empty
>    swap space is still enough.
>
> 3) dm-cache swap disk 1G, untar kernel.tgz to ext2 on zram-blk, build -j 4
>    dmcache consists of zram-meta 10M, zram-cache 1G and real swap storage 1G
>    No OOM happens and successfully building done.
>
> Above tests proves zram can support writeback into real swap storage
> so that zram-cache can always have a free space. If necessary, we could
> add new plugin in dm-cache. I see It's really flexible and well-layered
> architecure so zram-blk's concept is good for us and it has lots of
> potential to be enhanced by MM/FS/Block developers.
>
> As other disadvantage of zswap writeback, frontswap's semantic is
> synchronous API so zswap should decompress in memory zpage
> right before writeback and even, it writes pages one by one,
> not a batch. If we extend frontswap API, we would enhance it but
> I belive we can do better in device mapper layer which is aware of
> block align, bandwidth, mapping table, asynchronous and lots of hints
> from the block layer. Nonetheless, if we should merge zram's
> functionality to zswap, I think zram should include zswap's
> functionaliy(But I hope it will never happen) because old age zram
> already has lots of real users rather than new young zswap so it's
> more handy to unify them with keeping changelog which is one of
> valuable things getting from staging stay for a long time.
>
> The reason zram doesn't support writeback until now is just shortage
> of needs. The zram's main customers were embedded people so writeback
> into real swap storage is too bad for interactivity and wear-leveling
> on low falsh devices. But like above, zram has a potential to support
> writeback with other block drivers or more reasonable VM enhance
> so I'd like to claim zram's block concept is really good.
>
> Another zram-blk's usecase is following as.
> The admin can format /dev/zramX with any FS and mount on it.
> It could help small memory system, too. For exmaple, many embedded
> system don't have swap so although tmpfs can support swapout,
> it's pointless. Then, let's assume temp file growing up until half
> of system memory once in a while. We don't want to write it on flash
> by wear-leveing issue and response problem so we want to keep in-memory.
> But if we use tmpfs, it should evict half of working set to cover them
> when the size reach peak. In the case, zram-blk would be good fit, too.
>
> I'd like to enhance zram with more features like zsmalloc-compaction,
> , async I/O, parallel decompression and so on but zram developers cannot
> do it now because Greg, staging maintainer, doesn't want to add new feature
> until promotion is done because zram have been in staging for a very long time.
> Acutally, some patches about enhance are pending for a long time.
>
> [1] https://lkml.org/lkml/2013/8/21/141
>
>>
>> thanks,
>>
>> greg k-h
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>
> --
> Kind regards,
> Minchan Kim
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Luigi Semenzato]

On Thu, Nov 7, 2013 at 9:06 AM, Luigi Semenzato <semenzato@google.com> wrote:

-> Android 4.4 KitKat is also using zram, to better support devices with
-> less than 1 MB RAM.  (That's the news.)

Sorry, I meant 1 GB RAM.

http://dilbert.com/strips/comic/1991-09-27/

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Greg KH]

On Thu, Nov 07, 2013 at 09:06:26AM -0800, Luigi Semenzato wrote:
> If I may add my usual 2c (and some news):
> 
> zram is used by default on all Chrome OS devices.  I can't say how
> many devices, but it's not a small number, google it, and it's an
> important market, low-end laptops for education and the less affluent.
>  It has been available experimentally for well over a year.
> 
> Android 4.4 KitKat is also using zram, to better support devices with
> less than 1 MB RAM.  (That's the news.)
> 
> When comparing the relative advantages of the two subsystems (zram and
> zswap), let's not forget that considerable effort goes into in tuning
> and bug fixing for specific use cases---possibly even more than the
> initial development effort.  Zram has not just been sitting around in
> drivers/staging, it's in serious use.
> 
> If we were to judge systems based merely on theoretical technical
> merit, then we should consider switching en masse to FreeBSD.  (I said
> we should *consider* :).
> 
> I am very familiar with the limitations of zram, but it works well and
> I think it would be wise to keep supporting it.  Besides, it's small
> and AFAICT it interfaces cleanly with the rest of the system, so I
> don't see what the big deal is.

Then please help with getting it merged properly, and out of staging.

thanks,

greg k-h

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Rik van Riel]

On 11/07/2013 02:04 AM, Minchan Kim wrote:

> I'm guilty and I have been busy by other stuff. Sorry for that.
> Fortunately, I discussed this issue with Hugh in this Linuxcon for a
> long time(Thanks Hugh!) he felt zram's block device abstraction is
> better design rather than frontswap backend stuff although it's a question
> where we put zsmalloc. I will CC Hugh because many of things is related
> to swap subsystem and his opinion is really important.
> And I discussed it with Rik and he feel positive about zram.

To clarify that, I agree with Minchan that there are certain
workloads where zram is probably more appropriate than zswap.

For most of the workloads that I am interested in, zswap will
be more interesting, but zram seems to have its own niche, and
I certainly do not want to hold back the embedded folks...

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Bob Liu]

On 11/07/2013 03:04 PM, Minchan Kim wrote:
> On Wed, Nov 06, 2013 at 07:05:11PM -0800, Greg KH wrote:
>> On Wed, Nov 06, 2013 at 03:46:19PM -0800, Nitin Gupta wrote:
>>  > I'm getting really tired of them hanging around in here for many years
>>>> now...
>>>>
>>>
>>> Minchan has tried many times to promote zram out of staging. This was
>>> his most recent attempt:
>>>
>>> https://lkml.org/lkml/2013/8/21/54
>>>
>>> There he provided arguments for zram inclusion, how it can help in
>>> situations where zswap can't and why generalizing /dev/ramX would
>>> not be a great idea. So, cannot say why it wasn't picked up
>>> for inclusion at that time.
>>>
>>>> Should I just remove them if no one is working on getting them merged
>>>> "properly"?
>>>>
>>>
>>> Please refer the mail thread (link above) and see Minchan's
>>> justifications for zram.
>>> If they don't sound convincing enough then please remove zram+zsmalloc
>>> from staging.
>>
>> You don't need to be convincing me, you need to be convincing the
>> maintainers of the area of the kernel you are working with.
>>
>> And since the last time you all tried to get this merged was back in
>> August, I'm feeling that you all have given up, so it needs to be
>> deleted.  I'll go do that for 3.14, and if someone wants to pick it up
>> and merge it properly, they can easily revert it.
> 
> I'm guilty and I have been busy by other stuff. Sorry for that.
> Fortunately, I discussed this issue with Hugh in this Linuxcon for a
> long time(Thanks Hugh!) he felt zram's block device abstraction is
> better design rather than frontswap backend stuff although it's a question
> where we put zsmalloc. I will CC Hugh because many of things is related
> to swap subsystem and his opinion is really important.
> And I discussed it with Rik and he feel positive about zram.
> 
> Last impression Andrw gave me by private mail is he want to merge
> zram's functionality into zswap or vise versa.
> If I misunderstood, please correct me.
> I understand his concern but I guess he didn't have a time to read
> my long description due to a ton of works at that time.
> So, I will try one more time.
> I hope I'd like to listen feedback than *silence* so that we can
> move forward than stall.
> 
> Recently, Bob tried to move zsmalloc under mm directory to unify
> zram and zswap with adding pseudo block device in zswap(It's
> very weired to me. I think it's horrible monster which is lying
> between mm and block in layering POV) but he was ignoring zram's
> block device (a.k.a zram-blk) feature and considered only swap
> usecase of zram, in turn, it lose zram's good concept. 
> I already convered other topics Bob raised in this thread[1]
> and why I think zram is better in the thread.
> 

I have no objections for zram, and I also think is good for zswap can
support zsmalloc and fake swap device. At least users can have more
options just like slab/slub/slob.

-- 
Regards,
-Bob

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Minchan Kim]

On Thu, Nov 07, 2013 at 04:04:51PM +0900, Minchan Kim wrote:
> On Wed, Nov 06, 2013 at 07:05:11PM -0800, Greg KH wrote:
> > On Wed, Nov 06, 2013 at 03:46:19PM -0800, Nitin Gupta wrote:
> >  > I'm getting really tired of them hanging around in here for many years
> > > > now...
> > > >
> > > 
> > > Minchan has tried many times to promote zram out of staging. This was
> > > his most recent attempt:
> > > 
> > > https://lkml.org/lkml/2013/8/21/54
> > > 
> > > There he provided arguments for zram inclusion, how it can help in
> > > situations where zswap can't and why generalizing /dev/ramX would
> > > not be a great idea. So, cannot say why it wasn't picked up
> > > for inclusion at that time.
> > > 
> > > > Should I just remove them if no one is working on getting them merged
> > > > "properly"?
> > > >
> > > 
> > > Please refer the mail thread (link above) and see Minchan's
> > > justifications for zram.
> > > If they don't sound convincing enough then please remove zram+zsmalloc
> > > from staging.
> > 
> > You don't need to be convincing me, you need to be convincing the
> > maintainers of the area of the kernel you are working with.
> > 
> > And since the last time you all tried to get this merged was back in
> > August, I'm feeling that you all have given up, so it needs to be
> > deleted.  I'll go do that for 3.14, and if someone wants to pick it up
> > and merge it properly, they can easily revert it.
> 
> I'm guilty and I have been busy by other stuff. Sorry for that.
> Fortunately, I discussed this issue with Hugh in this Linuxcon for a
> long time(Thanks Hugh!) he felt zram's block device abstraction is
> better design rather than frontswap backend stuff although it's a question
> where we put zsmalloc. I will CC Hugh because many of things is related
> to swap subsystem and his opinion is really important.
> And I discussed it with Rik and he feel positive about zram.
> 
> Last impression Andrw gave me by private mail is he want to merge
> zram's functionality into zswap or vise versa.
> If I misunderstood, please correct me.
> I understand his concern but I guess he didn't have a time to read
> my long description due to a ton of works at that time.
> So, I will try one more time.
> I hope I'd like to listen feedback than *silence* so that we can
> move forward than stall.
> 
> Recently, Bob tried to move zsmalloc under mm directory to unify
> zram and zswap with adding pseudo block device in zswap(It's
> very weired to me. I think it's horrible monster which is lying
> between mm and block in layering POV) but he was ignoring zram's
> block device (a.k.a zram-blk) feature and considered only swap
> usecase of zram, in turn, it lose zram's good concept. 
> I already convered other topics Bob raised in this thread[1]
> and why I think zram is better in the thread.
> 
> Will repeat one more time and hope gray beards penguins grab a
> time in this time and they give a conclusion/direction to me so
> that we don't lose lots of user and functionality.
> 
> ========== &< ===========
> 
> Mel raised an another issue in v6, "maintainance headache".
> He claimed zswap and zram has a similar goal that is to compresss
> swap pages so if we promote zram, maintainance headache happens
> sometime by diverging implementaion between zswap and zram
> so that he want to unify zram and zswap. For it, he want zswap
> to implement pseudo block device like Bob did to emulate zram so
> zswap can have an advantage of writeback as well as zram's benefit.
> But I wonder frontswap-based zswap's writeback is really good
> approach for writeback POV. I think that problem isn't only
> specific for zswap. If we want to configure multiple swap hierarchy
> with various speed device such as RAM, NVRAM, SSD, eMMC, NAS etc,
> it would be a general problem. So we should think of more general
> approach. At a glance, I can see two approach.
> 
> First, VM could be aware of heterogeneous swap configuration
> so it could aim for being able to configure cache hierarchy
> among swap devices. It may need indirction layer on swap, which
> was already talked about that way so VM can migrate a block from
> A to B easily. It will support various configuration with VM's
> hints, maybe, in future.
> http://lkml.indiana.edu/hypermail/linux/kernel/1203.3/03812.html
> 
> Second, as more practical solution, we could use device mapper like
> dm-cache(https://lwn.net/Articles/540996/), which makes it very
> flexible. Now, it supports various configruation and cache policy
> (block size, writeback/writethrough, LRU, MFU although MQ is merged
> now) so it would be good fit for our purpose. Even, it can make zram
> support writeback. I tested it following as following scenario
> in KVM 4 CPU, 1G DRAM with background 800M memory hogger, which is
> allocates random data up to 800M.
> 
> 1) zram swap disk 1G, untar kernel.tgz to tmpfs, build -j 4
>    Fail to untar due to shortage of memory space by tmpfs default size limit
> 
> 2) zram swap disk 1G, untar kernel.tgz to ext2 on zram-blk, build -j 4
>    OOM happens while building the kernel but it untar successfully
>    on ext2 based on zram-blk. The reason OOM happend is zram can not find
>    free pages from main memory to store swap out pages although empty
>    swap space is still enough.
> 
> 3) dm-cache swap disk 1G, untar kernel.tgz to ext2 on zram-blk, build -j 4
>    dmcache consists of zram-meta 10M, zram-cache 1G and real swap storage 1G
>    No OOM happens and successfully building done.
> 
> Above tests proves zram can support writeback into real swap storage
> so that zram-cache can always have a free space. If necessary, we could
> add new plugin in dm-cache. I see It's really flexible and well-layered
> architecure so zram-blk's concept is good for us and it has lots of
> potential to be enhanced by MM/FS/Block developers.
> 
> As other disadvantage of zswap writeback, frontswap's semantic is
> synchronous API so zswap should decompress in memory zpage
> right before writeback and even, it writes pages one by one,
> not a batch. If we extend frontswap API, we would enhance it but
> I belive we can do better in device mapper layer which is aware of
> block align, bandwidth, mapping table, asynchronous and lots of hints
> from the block layer. Nonetheless, if we should merge zram's
> functionality to zswap, I think zram should include zswap's
> functionaliy(But I hope it will never happen) because old age zram
> already has lots of real users rather than new young zswap so it's
> more handy to unify them with keeping changelog which is one of
> valuable things getting from staging stay for a long time.
> 
> The reason zram doesn't support writeback until now is just shortage
> of needs. The zram's main customers were embedded people so writeback
> into real swap storage is too bad for interactivity and wear-leveling
> on low falsh devices. But like above, zram has a potential to support
> writeback with other block drivers or more reasonable VM enhance
> so I'd like to claim zram's block concept is really good.
> 
> Another zram-blk's usecase is following as.
> The admin can format /dev/zramX with any FS and mount on it.
> It could help small memory system, too. For exmaple, many embedded
> system don't have swap so although tmpfs can support swapout,
> it's pointless. Then, let's assume temp file growing up until half
> of system memory once in a while. We don't want to write it on flash
> by wear-leveing issue and response problem so we want to keep in-memory.
> But if we use tmpfs, it should evict half of working set to cover them
> when the size reach peak. In the case, zram-blk would be good fit, too.
> 
> I'd like to enhance zram with more features like zsmalloc-compaction,
> , async I/O, parallel decompression and so on but zram developers cannot
> do it now because Greg, staging maintainer, doesn't want to add new feature
> until promotion is done because zram have been in staging for a very long time.
> Acutally, some patches about enhance are pending for a long time.
> 
> [1] https://lkml.org/lkml/2013/8/21/141
> 

Hello Andrew,

I'd like to listen your opinion.

The zram promotion trial started since Aug 2012 and I already have get many
Acked/Reviewed feedback and positive feedback from Rik and Bob in this thread.
(ex, Jens Axboe[1], Konrad Rzeszutek Wilk[2], Nitin Gupta[3], Pekka Enberg[4])
In Linuxcon, Hugh gave positive feedback about zram(Hugh, If I misunderstood,
please correct me!). And there are lots of users already in embedded industry
ex, (most of TV in the world, Chromebook, CyanogenMod, Android Kitkat.)
They are not idiot. Zram is really effective for embedded world.

We spent much time with preventing zram enhance since it have been in staging
and Greg never want to improve without promotion.

Please consider promotion and let us improve it.
I think only remained thing is your decision.


1. https://lkml.org/lkml/2012/9/11/551
2. https://lkml.org/lkml/2012/8/9/636
3. https://lkml.org/lkml/2012/8/8/390
4. https://lkml.org/lkml/2012/9/26/126

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Greg KH]

On Wed, Nov 13, 2013 at 12:41:38AM +0900, Minchan Kim wrote:
> We spent much time with preventing zram enhance since it have been in staging
> and Greg never want to improve without promotion.

It's not "improve", it's "Greg does not want you adding new features and
functionality while the code is in staging."  I want you to spend your
time on getting it out of staging first.

Now if something needs to be done based on review and comments to the
code, then that's fine to do and I'll accept that, but I've been seeing
new functionality be added to the code, which I will not accept because
it seems that you all have given up on getting it merged, which isn't
ok.

thanks,

greg k-h

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Nitin Gupta]

On 11/12/13, 6:42 PM, Greg KH wrote:
> On Wed, Nov 13, 2013 at 12:41:38AM +0900, Minchan Kim wrote:
>> We spent much time with preventing zram enhance since it have been in staging
>> and Greg never want to improve without promotion.
>
> It's not "improve", it's "Greg does not want you adding new features and
> functionality while the code is in staging."  I want you to spend your
> time on getting it out of staging first.
>
> Now if something needs to be done based on review and comments to the
> code, then that's fine to do and I'll accept that, but I've been seeing
> new functionality be added to the code, which I will not accept because
> it seems that you all have given up on getting it merged, which isn't
> ok.
>

It's not that people have given up on getting it merged but every time 
patches are posted, there is really no response from maintainers perhaps 
due to their lack of interest in embedded, or perhaps they believe 
embedded folks are making a wrong choice by using zram. Either way, a 
final word, instead of just silence would be more helpful.

Thanks,
Nitin

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Hugh Dickins]

On Wed, 13 Nov 2013, Minchan Kim wrote:
> On Thu, Nov 07, 2013 at 04:04:51PM +0900, Minchan Kim wrote:
> > On Wed, Nov 06, 2013 at 07:05:11PM -0800, Greg KH wrote:
> > > On Wed, Nov 06, 2013 at 03:46:19PM -0800, Nitin Gupta wrote:
> > >  > I'm getting really tired of them hanging around in here for many years
> > > > > now...
> > > > >
> > > > 
> > > > Minchan has tried many times to promote zram out of staging. This was
> > > > his most recent attempt:
> > > > 
> > > > https://lkml.org/lkml/2013/8/21/54
...
> 
> Hello Andrew,
> 
> I'd like to listen your opinion.
> 
> The zram promotion trial started since Aug 2012 and I already have get many
> Acked/Reviewed feedback and positive feedback from Rik and Bob in this thread.
> (ex, Jens Axboe[1], Konrad Rzeszutek Wilk[2], Nitin Gupta[3], Pekka Enberg[4])
> In Linuxcon, Hugh gave positive feedback about zram(Hugh, If I misunderstood,
> please correct me!). And there are lots of users already in embedded industry
> ex, (most of TV in the world, Chromebook, CyanogenMod, Android Kitkat.)
> They are not idiot. Zram is really effective for embedded world.

Sorry for taking so long to respond, Minchan: no, you do not misrepresent
me at all.  Promotion of zram and zsmalloc from staging is way overdue:
they long ago proved their worth, look tidy, and have an active maintainer.

Putting them into drivers/staging was always a mistake, and I quite
understand Greg's impatience with them by now; but please let's move
them to where they belong instead of removing them.

I would not have lent support to zswap if I'd thought that was going to
block zram.  And I was not the only one surprised when zswap replaced its
use of zsmalloc by zbud: we had rather expected a zbud option to be added,
and I still assume that zsmalloc support will be added back to zswap later.

I think your August 2013 posting moved zsmalloc under zram and moved it
all to drivers/block?  That is the right place for zram, but I do think
zsmalloc.c (I'm not very keen on _drvs and -mains myself) should be
alongside zbud.c in mm, where we can better keep an eye on its
struct-pageyness.

IMHO
Hugh

> 
> We spent much time with preventing zram enhance since it have been in staging
> and Greg never want to improve without promotion.
> 
> Please consider promotion and let us improve it.
> I think only remained thing is your decision.
> 
> 
> 1. https://lkml.org/lkml/2012/9/11/551
> 2. https://lkml.org/lkml/2012/8/9/636
> 3. https://lkml.org/lkml/2012/8/8/390
> 4. https://lkml.org/lkml/2012/9/26/126

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Seth Jennings]

On Wed, Nov 13, 2013 at 08:00:34PM -0800, Hugh Dickins wrote:
> On Wed, 13 Nov 2013, Minchan Kim wrote:
> > On Thu, Nov 07, 2013 at 04:04:51PM +0900, Minchan Kim wrote:
> > > On Wed, Nov 06, 2013 at 07:05:11PM -0800, Greg KH wrote:
> > > > On Wed, Nov 06, 2013 at 03:46:19PM -0800, Nitin Gupta wrote:
> > > >  > I'm getting really tired of them hanging around in here for many years
> > > > > > now...
> > > > > >
> > > > > 
> > > > > Minchan has tried many times to promote zram out of staging. This was
> > > > > his most recent attempt:
> > > > > 
> > > > > https://lkml.org/lkml/2013/8/21/54
> ...
> > 
> > Hello Andrew,
> > 
> > I'd like to listen your opinion.
> > 
> > The zram promotion trial started since Aug 2012 and I already have get many
> > Acked/Reviewed feedback and positive feedback from Rik and Bob in this thread.
> > (ex, Jens Axboe[1], Konrad Rzeszutek Wilk[2], Nitin Gupta[3], Pekka Enberg[4])
> > In Linuxcon, Hugh gave positive feedback about zram(Hugh, If I misunderstood,
> > please correct me!). And there are lots of users already in embedded industry
> > ex, (most of TV in the world, Chromebook, CyanogenMod, Android Kitkat.)
> > They are not idiot. Zram is really effective for embedded world.
> 
> Sorry for taking so long to respond, Minchan: no, you do not misrepresent
> me at all.  Promotion of zram and zsmalloc from staging is way overdue:
> they long ago proved their worth, look tidy, and have an active maintainer.
> 
> Putting them into drivers/staging was always a mistake, and I quite
> understand Greg's impatience with them by now; but please let's move
> them to where they belong instead of removing them.
> 
> I would not have lent support to zswap if I'd thought that was going to
> block zram.  And I was not the only one surprised when zswap replaced its
> use of zsmalloc by zbud: we had rather expected a zbud option to be added,
> and I still assume that zsmalloc support will be added back to zswap later.

Yes, it is still the plan to reintroduce zsmalloc as an option (possibly
_the_ option) for zswap.

An idea being tossed around is making zswap writethrough instead of
delayed writeback.

Doing this would be mean that zswap would no longer reduce swap out
traffic, but would continue to reduce swap in latency by reading out of
the compressed cache instead of the swap device.

For that loss, we gain a benefit: the compressed pages in the cache are
clean, meaning we can reclaim them at any time with no writeback
cost.  This addresses Mel's initial concern (the one that led to zswap
moving to zbud) about writeback latency when the zswap pool is full.

If there is no writeback cost for reclaiming space in the compressed
pool, then we can use higher density packing like zsmalloc.

Making zswap writethough would also make the difference between zswap
and zram, both in terms of operation and application, more apparent,
demonstrating the need for both.

Seth

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Bob Liu]

On 11/15/2013 12:21 AM, Seth Jennings wrote:
> On Wed, Nov 13, 2013 at 08:00:34PM -0800, Hugh Dickins wrote:
>> On Wed, 13 Nov 2013, Minchan Kim wrote:
>> ...
>>>
>>> Hello Andrew,
>>>
>>> I'd like to listen your opinion.
>>>
>>> The zram promotion trial started since Aug 2012 and I already have get many
>>> Acked/Reviewed feedback and positive feedback from Rik and Bob in this thread.
>>> (ex, Jens Axboe[1], Konrad Rzeszutek Wilk[2], Nitin Gupta[3], Pekka Enberg[4])
>>> In Linuxcon, Hugh gave positive feedback about zram(Hugh, If I misunderstood,
>>> please correct me!). And there are lots of users already in embedded industry
>>> ex, (most of TV in the world, Chromebook, CyanogenMod, Android Kitkat.)
>>> They are not idiot. Zram is really effective for embedded world.
>>
>> Sorry for taking so long to respond, Minchan: no, you do not misrepresent
>> me at all.  Promotion of zram and zsmalloc from staging is way overdue:
>> they long ago proved their worth, look tidy, and have an active maintainer.
>>
>> Putting them into drivers/staging was always a mistake, and I quite
>> understand Greg's impatience with them by now; but please let's move
>> them to where they belong instead of removing them.
>>
>> I would not have lent support to zswap if I'd thought that was going to
>> block zram.  And I was not the only one surprised when zswap replaced its
>> use of zsmalloc by zbud: we had rather expected a zbud option to be added,
>> and I still assume that zsmalloc support will be added back to zswap later.
> 
> Yes, it is still the plan to reintroduce zsmalloc as an option (possibly
> _the_ option) for zswap.
> 
> An idea being tossed around is making zswap writethrough instead of
> delayed writeback.
>
> Doing this would be mean that zswap would no longer reduce swap out
> traffic, but would continue to reduce swap in latency by reading out of
> the compressed cache instead of the swap device.
> 
> For that loss, we gain a benefit: the compressed pages in the cache are
> clean, meaning we can reclaim them at any time with no writeback
> cost.  This addresses Mel's initial concern (the one that led to zswap
> moving to zbud) about writeback latency when the zswap pool is full.
> 

Agree!

> If there is no writeback cost for reclaiming space in the compressed
> pool, then we can use higher density packing like zsmalloc.
> 

But zsmalloc will compact several 0-order pages together as a zpage
which cause it not easy to reclaim one 0-order page directly from it.
Especially if we want to make zswap pool can be dynamically managed in
future.

> Making zswap writethough would also make the difference between zswap
> and zram, both in terms of operation and application, more apparent,
> demonstrating the need for both.
> 

-- 
Regards,
-Bob

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Minchan Kim]

Hello Hugh,

On Wed, Nov 13, 2013 at 08:00:34PM -0800, Hugh Dickins wrote:
> On Wed, 13 Nov 2013, Minchan Kim wrote:
> > On Thu, Nov 07, 2013 at 04:04:51PM +0900, Minchan Kim wrote:
> > > On Wed, Nov 06, 2013 at 07:05:11PM -0800, Greg KH wrote:
> > > > On Wed, Nov 06, 2013 at 03:46:19PM -0800, Nitin Gupta wrote:
> > > >  > I'm getting really tired of them hanging around in here for many years
> > > > > > now...
> > > > > >
> > > > > 
> > > > > Minchan has tried many times to promote zram out of staging. This was
> > > > > his most recent attempt:
> > > > > 
> > > > > https://lkml.org/lkml/2013/8/21/54
> ...
> > 
> > Hello Andrew,
> > 
> > I'd like to listen your opinion.
> > 
> > The zram promotion trial started since Aug 2012 and I already have get many
> > Acked/Reviewed feedback and positive feedback from Rik and Bob in this thread.
> > (ex, Jens Axboe[1], Konrad Rzeszutek Wilk[2], Nitin Gupta[3], Pekka Enberg[4])
> > In Linuxcon, Hugh gave positive feedback about zram(Hugh, If I misunderstood,
> > please correct me!). And there are lots of users already in embedded industry
> > ex, (most of TV in the world, Chromebook, CyanogenMod, Android Kitkat.)
> > They are not idiot. Zram is really effective for embedded world.
> 
> Sorry for taking so long to respond, Minchan: no, you do not misrepresent
> me at all.  Promotion of zram and zsmalloc from staging is way overdue:
> they long ago proved their worth, look tidy, and have an active maintainer.
> 
> Putting them into drivers/staging was always a mistake, and I quite
> understand Greg's impatience with them by now; but please let's move
> them to where they belong instead of removing them.
> 
> I would not have lent support to zswap if I'd thought that was going to
> block zram.  And I was not the only one surprised when zswap replaced its
> use of zsmalloc by zbud: we had rather expected a zbud option to be added,
> and I still assume that zsmalloc support will be added back to zswap later.
> 
> I think your August 2013 posting moved zsmalloc under zram and moved it
> all to drivers/block?  That is the right place for zram, but I do think
> zsmalloc.c (I'm not very keen on _drvs and -mains myself) should be
> alongside zbud.c in mm, where we can better keep an eye on its
> struct-pageyness.

It's really no problem and it was what I want from the beginning.
https://lkml.org/lkml/2012/9/11/551
I will do in next posting.

Before that, I'd like to listen Andrew's opinion about promoting because
my previous trials to promote zram have ignored so it was just waste
for my time and noisy for you guys. 

Andrew, please tell us your decision. May I go ahead?

> 
> IMHO
> Hugh
> 
> > 
> > We spent much time with preventing zram enhance since it have been in staging
> > and Greg never want to improve without promotion.
> > 
> > Please consider promotion and let us improve it.
> > I think only remained thing is your decision.
> > 
> > 
> > 1. https://lkml.org/lkml/2012/9/11/551
> > 2. https://lkml.org/lkml/2012/8/9/636
> > 3. https://lkml.org/lkml/2012/8/8/390
> > 4. https://lkml.org/lkml/2012/9/26/126
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Kind regards,
Minchan Kim

[PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Olav Haugan]

zsmalloc encodes a handle using the page pfn and an object
index. On some hardware platforms the pfn could be 0 and this
causes the encoded handle to be 0 which is interpreted as an
allocation failure.

To prevent this false error we ensure that the encoded handle
will not be 0 when allocation succeeds.

Change-Id: Ifff930dcf254915b497aec5cb36f152a5e5365d6
Signed-off-by: Olav Haugan <ohaugan@codeaurora.org>
---
 drivers/staging/zsmalloc/zsmalloc-main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c b/drivers/staging/zsmalloc/zsmalloc-main.c
index 523b937..0e32c0f 100644
--- a/drivers/staging/zsmalloc/zsmalloc-main.c
+++ b/drivers/staging/zsmalloc/zsmalloc-main.c
@@ -441,7 +441,7 @@ static void *obj_location_to_handle(struct page *page, unsigned long obj_idx)
 	}
 
 	handle = page_to_pfn(page) << OBJ_INDEX_BITS;
-	handle |= (obj_idx & OBJ_INDEX_MASK);
+	handle |= ((obj_idx + 1) & OBJ_INDEX_MASK);
 
 	return (void *)handle;
 }
@@ -451,7 +451,7 @@ static void obj_handle_to_location(unsigned long handle, struct page **page,
 				unsigned long *obj_idx)
 {
 	*page = pfn_to_page(handle >> OBJ_INDEX_BITS);
-	*obj_idx = handle & OBJ_INDEX_MASK;
+	*obj_idx = (handle & OBJ_INDEX_MASK) - 1;
 }
 
 static unsigned long obj_idx_to_offset(struct page *page,
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[David Cohen]

Hi Olav,

On 11/05/2013 04:54 PM, Olav Haugan wrote:
> zsmalloc encodes a handle using the page pfn and an object
> index. On some hardware platforms the pfn could be 0 and this
> causes the encoded handle to be 0 which is interpreted as an
> allocation failure.
>
> To prevent this false error we ensure that the encoded handle
> will not be 0 when allocation succeeds.
>
> Change-Id: Ifff930dcf254915b497aec5cb36f152a5e5365d6
> Signed-off-by: Olav Haugan <ohaugan@codeaurora.org>
> ---
>   drivers/staging/zsmalloc/zsmalloc-main.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c b/drivers/staging/zsmalloc/zsmalloc-main.c
> index 523b937..0e32c0f 100644
> --- a/drivers/staging/zsmalloc/zsmalloc-main.c
> +++ b/drivers/staging/zsmalloc/zsmalloc-main.c
> @@ -441,7 +441,7 @@ static void *obj_location_to_handle(struct page *page, unsigned long obj_idx)
>   	}
>
>   	handle = page_to_pfn(page) << OBJ_INDEX_BITS;
> -	handle |= (obj_idx & OBJ_INDEX_MASK);
> +	handle |= ((obj_idx + 1) & OBJ_INDEX_MASK);

As suggestion you could use a macro instead of hardcoded 1.

I am not familiar with this code, but if it's a valid test to verify if
the resulting address is page aligned, you might want to set this
offset macro to a page aligned value as well.

>
>   	return (void *)handle;
>   }
> @@ -451,7 +451,7 @@ static void obj_handle_to_location(unsigned long handle, struct page **page,
>   				unsigned long *obj_idx)
>   {
>   	*page = pfn_to_page(handle >> OBJ_INDEX_BITS);
> -	*obj_idx = handle & OBJ_INDEX_MASK;
> +	*obj_idx = (handle & OBJ_INDEX_MASK) - 1;

Ditto.

Br, David Cohen

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Nitin Gupta]

On Tue, Nov 5, 2013 at 5:17 PM, David Cohen
<david.a.cohen@linux.intel.com> wrote:
> Hi Olav,
>
>
> On 11/05/2013 04:54 PM, Olav Haugan wrote:
>>
>> zsmalloc encodes a handle using the page pfn and an object
>> index. On some hardware platforms the pfn could be 0 and this
>> causes the encoded handle to be 0 which is interpreted as an
>> allocation failure.
>>
>> To prevent this false error we ensure that the encoded handle
>> will not be 0 when allocation succeeds.
>>
>> Change-Id: Ifff930dcf254915b497aec5cb36f152a5e5365d6
>> Signed-off-by: Olav Haugan <ohaugan@codeaurora.org>
>> ---
>>   drivers/staging/zsmalloc/zsmalloc-main.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c
>> b/drivers/staging/zsmalloc/zsmalloc-main.c
>> index 523b937..0e32c0f 100644
>> --- a/drivers/staging/zsmalloc/zsmalloc-main.c
>> +++ b/drivers/staging/zsmalloc/zsmalloc-main.c
>> @@ -441,7 +441,7 @@ static void *obj_location_to_handle(struct page *page,
>> unsigned long obj_idx)
>>         }
>>
>>         handle = page_to_pfn(page) << OBJ_INDEX_BITS;
>> -       handle |= (obj_idx & OBJ_INDEX_MASK);
>> +       handle |= ((obj_idx + 1) & OBJ_INDEX_MASK);
>
>
> As suggestion you could use a macro instead of hardcoded 1.
>
> I am not familiar with this code, but if it's a valid test to verify if
> the resulting address is page aligned, you might want to set this
> offset macro to a page aligned value as well.
>
>

Using a hardcoded 1 looks fine in this case. But the patch description
should also be added as a comment for this function. Otherwise, the patch
looks good to me.

>>
>>         return (void *)handle;
>>   }
>> @@ -451,7 +451,7 @@ static void obj_handle_to_location(unsigned long
>> handle, struct page **page,
>>                                 unsigned long *obj_idx)
>>   {
>>         *page = pfn_to_page(handle >> OBJ_INDEX_BITS);
>> -       *obj_idx = handle & OBJ_INDEX_MASK;
>> +       *obj_idx = (handle & OBJ_INDEX_MASK) - 1;
>
>
> Ditto.
>

Thanks,
Nitin

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Olav Haugan]

On 11/6/2013 12:56 PM, Nitin Gupta wrote:
> On Tue, Nov 5, 2013 at 5:17 PM, David Cohen
> <david.a.cohen@linux.intel.com> wrote:
>> Hi Olav,
>>
>>
>> On 11/05/2013 04:54 PM, Olav Haugan wrote:
>>>
>>> zsmalloc encodes a handle using the page pfn and an object
>>> index. On some hardware platforms the pfn could be 0 and this
>>> causes the encoded handle to be 0 which is interpreted as an
>>> allocation failure.
>>>
>>> To prevent this false error we ensure that the encoded handle
>>> will not be 0 when allocation succeeds.
>>>
>>> Change-Id: Ifff930dcf254915b497aec5cb36f152a5e5365d6
>>> Signed-off-by: Olav Haugan <ohaugan@codeaurora.org>
>>> ---
>>>   drivers/staging/zsmalloc/zsmalloc-main.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c
>>> b/drivers/staging/zsmalloc/zsmalloc-main.c
>>> index 523b937..0e32c0f 100644
>>> --- a/drivers/staging/zsmalloc/zsmalloc-main.c
>>> +++ b/drivers/staging/zsmalloc/zsmalloc-main.c
>>> @@ -441,7 +441,7 @@ static void *obj_location_to_handle(struct page *page,
>>> unsigned long obj_idx)
>>>         }
>>>
>>>         handle = page_to_pfn(page) << OBJ_INDEX_BITS;
>>> -       handle |= (obj_idx & OBJ_INDEX_MASK);
>>> +       handle |= ((obj_idx + 1) & OBJ_INDEX_MASK);
>>
>>
>> As suggestion you could use a macro instead of hardcoded 1.
>>
>> I am not familiar with this code, but if it's a valid test to verify if
>> the resulting address is page aligned, you might want to set this
>> offset macro to a page aligned value as well.
>>
>>
> 
> Using a hardcoded 1 looks fine in this case. But the patch description
> should also be added as a comment for this function. Otherwise, the patch
> looks good to me.
> 

Sure, I can add a comment above obj_location_to_handle() and
obj_handle_to_location().


Olav Haugan

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Greg KH]

On Tue, Nov 05, 2013 at 04:54:12PM -0800, Olav Haugan wrote:
> zsmalloc encodes a handle using the page pfn and an object
> index. On some hardware platforms the pfn could be 0 and this
> causes the encoded handle to be 0 which is interpreted as an
> allocation failure.

What platforms specifically have this issue?

> 
> To prevent this false error we ensure that the encoded handle
> will not be 0 when allocation succeeds.
> 
> Change-Id: Ifff930dcf254915b497aec5cb36f152a5e5365d6

What is this?  What can anyone do with it?

> Signed-off-by: Olav Haugan <ohaugan@codeaurora.org>
> ---
>  drivers/staging/zsmalloc/zsmalloc-main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c b/drivers/staging/zsmalloc/zsmalloc-main.c
> index 523b937..0e32c0f 100644
> --- a/drivers/staging/zsmalloc/zsmalloc-main.c
> +++ b/drivers/staging/zsmalloc/zsmalloc-main.c
> @@ -441,7 +441,7 @@ static void *obj_location_to_handle(struct page *page, unsigned long obj_idx)
>  	}
>  
>  	handle = page_to_pfn(page) << OBJ_INDEX_BITS;
> -	handle |= (obj_idx & OBJ_INDEX_MASK);
> +	handle |= ((obj_idx + 1) & OBJ_INDEX_MASK);
>  
>  	return (void *)handle;
>  }
> @@ -451,7 +451,7 @@ static void obj_handle_to_location(unsigned long handle, struct page **page,
>  				unsigned long *obj_idx)
>  {
>  	*page = pfn_to_page(handle >> OBJ_INDEX_BITS);
> -	*obj_idx = handle & OBJ_INDEX_MASK;
> +	*obj_idx = (handle & OBJ_INDEX_MASK) - 1;
>  }

I need someone who knows how to test this code to ack it before I can
take it...

And I thought we were deleting zsmalloc anyway, why are you using this
code?  Isn't it no longer needed anymore?

greg k-h

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Nitin Gupta]

On Tue, Nov 5, 2013 at 5:56 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, Nov 05, 2013 at 04:54:12PM -0800, Olav Haugan wrote:
>> zsmalloc encodes a handle using the page pfn and an object
>> index. On some hardware platforms the pfn could be 0 and this
>> causes the encoded handle to be 0 which is interpreted as an
>> allocation failure.
>
> What platforms specifically have this issue?
>
>>
>> To prevent this false error we ensure that the encoded handle
>> will not be 0 when allocation succeeds.
>>
>> Change-Id: Ifff930dcf254915b497aec5cb36f152a5e5365d6
>
> What is this?  What can anyone do with it?
>
>> Signed-off-by: Olav Haugan <ohaugan@codeaurora.org>
>> ---
>>  drivers/staging/zsmalloc/zsmalloc-main.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c b/drivers/staging/zsmalloc/zsmalloc-main.c
>> index 523b937..0e32c0f 100644
>> --- a/drivers/staging/zsmalloc/zsmalloc-main.c
>> +++ b/drivers/staging/zsmalloc/zsmalloc-main.c
>> @@ -441,7 +441,7 @@ static void *obj_location_to_handle(struct page *page, unsigned long obj_idx)
>>       }
>>
>>       handle = page_to_pfn(page) << OBJ_INDEX_BITS;
>> -     handle |= (obj_idx & OBJ_INDEX_MASK);
>> +     handle |= ((obj_idx + 1) & OBJ_INDEX_MASK);
>>
>>       return (void *)handle;
>>  }
>> @@ -451,7 +451,7 @@ static void obj_handle_to_location(unsigned long handle, struct page **page,
>>                               unsigned long *obj_idx)
>>  {
>>       *page = pfn_to_page(handle >> OBJ_INDEX_BITS);
>> -     *obj_idx = handle & OBJ_INDEX_MASK;
>> +     *obj_idx = (handle & OBJ_INDEX_MASK) - 1;
>>  }
>
> I need someone who knows how to test this code to ack it before I can
> take it...
>
> And I thought we were deleting zsmalloc anyway, why are you using this
> code?  Isn't it no longer needed anymore?
>

zsmalloc is used by zram. Other zstuff has switched to zbud since they
need to do shrinking which is much easier to implement with simpler
design of zbud. For zram, which is a block device, we don't do such
active shrinking, so uses zsmalloc which provides much better density.

Nitin

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Greg KH]

On Wed, Nov 06, 2013 at 01:09:59PM -0800, Nitin Gupta wrote:
> On Tue, Nov 5, 2013 at 5:56 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Tue, Nov 05, 2013 at 04:54:12PM -0800, Olav Haugan wrote:
> >> zsmalloc encodes a handle using the page pfn and an object
> >> index. On some hardware platforms the pfn could be 0 and this
> >> causes the encoded handle to be 0 which is interpreted as an
> >> allocation failure.
> >
> > What platforms specifically have this issue?
> >
> >>
> >> To prevent this false error we ensure that the encoded handle
> >> will not be 0 when allocation succeeds.
> >>
> >> Change-Id: Ifff930dcf254915b497aec5cb36f152a5e5365d6
> >
> > What is this?  What can anyone do with it?
> >
> >> Signed-off-by: Olav Haugan <ohaugan@codeaurora.org>
> >> ---
> >>  drivers/staging/zsmalloc/zsmalloc-main.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c b/drivers/staging/zsmalloc/zsmalloc-main.c
> >> index 523b937..0e32c0f 100644
> >> --- a/drivers/staging/zsmalloc/zsmalloc-main.c
> >> +++ b/drivers/staging/zsmalloc/zsmalloc-main.c
> >> @@ -441,7 +441,7 @@ static void *obj_location_to_handle(struct page *page, unsigned long obj_idx)
> >>       }
> >>
> >>       handle = page_to_pfn(page) << OBJ_INDEX_BITS;
> >> -     handle |= (obj_idx & OBJ_INDEX_MASK);
> >> +     handle |= ((obj_idx + 1) & OBJ_INDEX_MASK);
> >>
> >>       return (void *)handle;
> >>  }
> >> @@ -451,7 +451,7 @@ static void obj_handle_to_location(unsigned long handle, struct page **page,
> >>                               unsigned long *obj_idx)
> >>  {
> >>       *page = pfn_to_page(handle >> OBJ_INDEX_BITS);
> >> -     *obj_idx = handle & OBJ_INDEX_MASK;
> >> +     *obj_idx = (handle & OBJ_INDEX_MASK) - 1;
> >>  }
> >
> > I need someone who knows how to test this code to ack it before I can
> > take it...
> >
> > And I thought we were deleting zsmalloc anyway, why are you using this
> > code?  Isn't it no longer needed anymore?
> >
> 
> zsmalloc is used by zram. Other zstuff has switched to zbud since they
> need to do shrinking which is much easier to implement with simpler
> design of zbud. For zram, which is a block device, we don't do such
> active shrinking, so uses zsmalloc which provides much better density.

Ok, so what's the plan of getting these other things out of staging?
I'm getting really tired of them hanging around in here for many years
now...

Should I just remove them if no one is working on getting them merged
"properly"?

thanks,

greg k-h

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Nitin Gupta]

On Wed, Nov 6, 2013 at 2:10 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Nov 06, 2013 at 01:09:59PM -0800, Nitin Gupta wrote:
>> On Tue, Nov 5, 2013 at 5:56 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Tue, Nov 05, 2013 at 04:54:12PM -0800, Olav Haugan wrote:
>> >> zsmalloc encodes a handle using the page pfn and an object
>> >> index. On some hardware platforms the pfn could be 0 and this
>> >> causes the encoded handle to be 0 which is interpreted as an
>> >> allocation failure.
>> >
>> > What platforms specifically have this issue?
>> >
>> >>
>> >> To prevent this false error we ensure that the encoded handle
>> >> will not be 0 when allocation succeeds.
>> >>
>> >> Change-Id: Ifff930dcf254915b497aec5cb36f152a5e5365d6
>> >
>> > What is this?  What can anyone do with it?
>> >
>> >> Signed-off-by: Olav Haugan <ohaugan@codeaurora.org>
>> >> ---
>> >>  drivers/staging/zsmalloc/zsmalloc-main.c | 4 ++--
>> >>  1 file changed, 2 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c b/drivers/staging/zsmalloc/zsmalloc-main.c
>> >> index 523b937..0e32c0f 100644
>> >> --- a/drivers/staging/zsmalloc/zsmalloc-main.c
>> >> +++ b/drivers/staging/zsmalloc/zsmalloc-main.c
>> >> @@ -441,7 +441,7 @@ static void *obj_location_to_handle(struct page *page, unsigned long obj_idx)
>> >>       }
>> >>
>> >>       handle = page_to_pfn(page) << OBJ_INDEX_BITS;
>> >> -     handle |= (obj_idx & OBJ_INDEX_MASK);
>> >> +     handle |= ((obj_idx + 1) & OBJ_INDEX_MASK);
>> >>
>> >>       return (void *)handle;
>> >>  }
>> >> @@ -451,7 +451,7 @@ static void obj_handle_to_location(unsigned long handle, struct page **page,
>> >>                               unsigned long *obj_idx)
>> >>  {
>> >>       *page = pfn_to_page(handle >> OBJ_INDEX_BITS);
>> >> -     *obj_idx = handle & OBJ_INDEX_MASK;
>> >> +     *obj_idx = (handle & OBJ_INDEX_MASK) - 1;
>> >>  }
>> >
>> > I need someone who knows how to test this code to ack it before I can
>> > take it...
>> >
>> > And I thought we were deleting zsmalloc anyway, why are you using this
>> > code?  Isn't it no longer needed anymore?
>> >
>>
>> zsmalloc is used by zram. Other zstuff has switched to zbud since they
>> need to do shrinking which is much easier to implement with simpler
>> design of zbud. For zram, which is a block device, we don't do such
>> active shrinking, so uses zsmalloc which provides much better density.
>
> Ok, so what's the plan of getting these other things out of staging?

Other zstuff: zswap and zcache

1) zswap (along with zbud allocator, frontcache, cleancache) is
already out of staging into mm/ (by Seth Jennings)
2) zcache seems to have been completely removed (not sure if Dan ever
wants to reintroduce it)

> I'm getting really tired of them hanging around in here for many years
> now...
>

Minchan has tried many times to promote zram out of staging. This was
his most recent attempt:

https://lkml.org/lkml/2013/8/21/54

There he provided arguments for zram inclusion, how it can help in
situations where zswap can't and why generalizing /dev/ramX would
not be a great idea. So, cannot say why it wasn't picked up
for inclusion at that time.

> Should I just remove them if no one is working on getting them merged
> "properly"?
>

Please refer the mail thread (link above) and see Minchan's
justifications for zram.
If they don't sound convincing enough then please remove zram+zsmalloc
from staging.

Nitin

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Greg KH]

On Wed, Nov 06, 2013 at 03:46:19PM -0800, Nitin Gupta wrote:
 > I'm getting really tired of them hanging around in here for many years
> > now...
> >
> 
> Minchan has tried many times to promote zram out of staging. This was
> his most recent attempt:
> 
> https://lkml.org/lkml/2013/8/21/54
> 
> There he provided arguments for zram inclusion, how it can help in
> situations where zswap can't and why generalizing /dev/ramX would
> not be a great idea. So, cannot say why it wasn't picked up
> for inclusion at that time.
> 
> > Should I just remove them if no one is working on getting them merged
> > "properly"?
> >
> 
> Please refer the mail thread (link above) and see Minchan's
> justifications for zram.
> If they don't sound convincing enough then please remove zram+zsmalloc
> from staging.

You don't need to be convincing me, you need to be convincing the
maintainers of the area of the kernel you are working with.

And since the last time you all tried to get this merged was back in
August, I'm feeling that you all have given up, so it needs to be
deleted.  I'll go do that for 3.14, and if someone wants to pick it up
and merge it properly, they can easily revert it.

thanks,

greg k-h

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Olav Haugan]

On 11/5/2013 5:56 PM, Greg KH wrote:
> On Tue, Nov 05, 2013 at 04:54:12PM -0800, Olav Haugan wrote:
>> zsmalloc encodes a handle using the page pfn and an object
>> index. On some hardware platforms the pfn could be 0 and this
>> causes the encoded handle to be 0 which is interpreted as an
>> allocation failure.
> 
> What platforms specifically have this issue?

Currently some of Qualcomm SoC's have physical memory that starts at
address 0x0 which causes this problem. I believe this could be a problem
on any platforms if memory is configured to be starting at physical
address 0x0 for these platforms.

>>
>> To prevent this false error we ensure that the encoded handle
>> will not be 0 when allocation succeeds.
>>
>> Change-Id: Ifff930dcf254915b497aec5cb36f152a5e5365d6
> 
> What is this?  What can anyone do with it?

This is an identifier used by "Gerrit Code Review" to track changes to
the same patch. I will remove it.

Olav Haugan

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Greg KH]

On Wed, Nov 06, 2013 at 04:00:02PM -0800, Olav Haugan wrote:
> On 11/5/2013 5:56 PM, Greg KH wrote:
> > On Tue, Nov 05, 2013 at 04:54:12PM -0800, Olav Haugan wrote:
> >> zsmalloc encodes a handle using the page pfn and an object
> >> index. On some hardware platforms the pfn could be 0 and this
> >> causes the encoded handle to be 0 which is interpreted as an
> >> allocation failure.
> > 
> > What platforms specifically have this issue?
> 
> Currently some of Qualcomm SoC's have physical memory that starts at
> address 0x0 which causes this problem.

Then say this, and list the exact SoC's that can have this problem so
people know how to evaluate the bugfix and see if it is relevant for
their systems.

> I believe this could be a problem
> on any platforms if memory is configured to be starting at physical
> address 0x0 for these platforms.

Have you seen this be a problem?  So it's just a theoretical issue at
this point in time?

> >> To prevent this false error we ensure that the encoded handle
> >> will not be 0 when allocation succeeds.
> >>
> >> Change-Id: Ifff930dcf254915b497aec5cb36f152a5e5365d6
> > 
> > What is this?  What can anyone do with it?
> 
> This is an identifier used by "Gerrit Code Review" to track changes to
> the same patch. I will remove it.

Please do so, it has no place in kernel patches submitted for
acceptance.

Please fix up the changelog, and the rest based on the other comments
and resend.

thanks,

greg k-h

Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Olav Haugan]

On 11/6/2013 7:06 PM, Greg KH wrote:
> On Wed, Nov 06, 2013 at 04:00:02PM -0800, Olav Haugan wrote:
>> On 11/5/2013 5:56 PM, Greg KH wrote:
>>> On Tue, Nov 05, 2013 at 04:54:12PM -0800, Olav Haugan wrote:
>>>> zsmalloc encodes a handle using the page pfn and an object
>>>> index. On some hardware platforms the pfn could be 0 and this
>>>> causes the encoded handle to be 0 which is interpreted as an
>>>> allocation failure.
>>>
>>> What platforms specifically have this issue?
>>
>> Currently some of Qualcomm SoC's have physical memory that starts at
>> address 0x0 which causes this problem.
> 
> Then say this, and list the exact SoC's that can have this problem so
> people know how to evaluate the bugfix and see if it is relevant for
> their systems.
> 
>> I believe this could be a problem
>> on any platforms if memory is configured to be starting at physical
>> address 0x0 for these platforms.
> 
> Have you seen this be a problem?  So it's just a theoretical issue at
> this point in time?

Yes, I can consistently reproduce it. It is not just theoretical.

Thanks,

Olav Haugan

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation