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

2013-11-08 Thread Nitin Gupta 
On Thu, Nov 7, 2013 at 5:58 PM, Olav Haugan ohau...@codeaurora.org wrote:
 zsmalloc encodes a handle using the pfn and an object
 index. On hardware platforms with physical memory starting
 at 0x0 the pfn can be 0. This causes the encoded handle to be
 0 and is incorrectly interpreted as an allocation failure.

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

 Signed-off-by: Olav Haugan ohau...@codeaurora.org
 ---
  drivers/staging/zsmalloc/zsmalloc-main.c | 17 +
  1 file changed, 13 insertions(+), 4 deletions(-)

 diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c 
 b/drivers/staging/zsmalloc/zsmalloc-main.c
 index 1a67537..3b950e5 100644
 --- a/drivers/staging/zsmalloc/zsmalloc-main.c
 +++ b/drivers/staging/zsmalloc/zsmalloc-main.c
 @@ -430,7 +430,12 @@ static struct page *get_next_page(struct page *page)
 return next;
  }

 -/* Encode page, obj_idx as a single handle value */
 +/*
 + * Encode page, obj_idx as a single handle value.
 + * On hardware platforms with physical memory starting at 0x0 the pfn
 + * could be 0 so we ensure that the handle will never be 0 by adjusting the
 + * encoded obj_idx value before encoding.
 + */
  static void *obj_location_to_handle(struct page *page, unsigned long obj_idx)
  {
 unsigned long handle;
 @@ -441,17 +446,21 @@ 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;
  }

 -/* Decode page, obj_idx pair from the given object handle */
 +/*
 + * Decode page, obj_idx pair from the given object handle. We adjust the
 + * decoded obj_idx back to its original value since it was adjusted in
 + * obj_location_to_handle().
 + */
  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,

Acked-by: Nitin Gupta ngu...@vflare.org

Thanks,
Nitin
--
To unsubscribe from this list: send the line unsubscribe linux-arm-msm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-11-06 Thread Nitin Gupta 
On Wed, Nov 6, 2013 at 2:10 PM, Greg KH gre...@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 gre...@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 ohau...@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
--
To unsubscribe from this list: send the line unsubscribe linux-arm-msm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html