RE: [PATCH 3/4] zsmalloc: add details to zs_map_object boiler plate
> From: Dan Magenheimer
> Subject: RE: [PATCH 3/4] zsmalloc: add details to zs_map_object boiler plate
>
> > From: Minchan Kim [mailto:minc...@kernel.org]
> > Subject: Re: [PATCH 3/4] zsmalloc: add details to zs_map_object boiler plate
> >
> > On Wed, Jul 11, 2012 at 09:15:43AM -0500, Seth Jennings wrote:
> > > On 07/11/2012 02:42 AM, Minchan Kim wrote:
> > > > On 07/11/2012 12:17 AM, Seth Jennings wrote:
> > > >> On 07/09/2012 09:35 PM, Minchan Kim wrote:
> > > >>> Maybe we need local_irq_save/restore in zs_[un]map_object path.
> > > >>
> > > >> I'd rather not disable interrupts since that will create
> > > >> unnecessary interrupt latency for all users, even if they
> > > >
> > > > Agreed.
> > > > Although we guide k[un]map atomic is so fast, it isn't necessary
> > > > to force irq_[enable|disable]. Okay.
> > > >
> > > >> don't need interrupt protection. If a particular user uses
> > > >> zs_map_object() in an interrupt path, it will be up to that
> > > >> user to disable interrupts to ensure safety.
> > > >
> > > > Nope. It shouldn't do that.
> > > > Any user in interrupt context can't assume that there isn't any other
> > > > user using per-cpu buffer
> > > > right before interrupt happens.
> > > >
> > > > The concern is that if such bug happens, it's very hard to find a bug.
> > > > So, how about adding this?
> > > >
> > > > void zs_map_object(...)
> > > > {
> > > > BUG_ON(in_interrupt());
> > > > }
> > >
> > > I not completely following you, but I think I'm following
> > > enough. Your point is that the per-cpu buffers are shared
> > > by all zsmalloc users and one user doesn't know if another
> > > user is doing a zs_map_object() in an interrupt path.
> >
> > And vise versa is yes.
> >
> > > However, I think what you are suggesting is to disallow
> > > mapping in interrupt context. This is a problem for zcache
> > > as it already does mapping in interrupt context, namely for
> > > page decompression in the page fault handler.
> >
> > I don't get it.
> > Page fault handler isn't interrupt context.
> >
> > > What do you think about making the per-cpu buffers local to
> > > each zsmalloc pool? That way each user has their own per-cpu
> > > buffers and don't step on each other's toes.
> >
> > Maybe, It could be a solution if you really need it in interrupt context.
> > But the concern is it could hurt zsmalloc's goal which is memory
> > space efficiency if your system has lots of CPUs.
>
> Sorry to be so far behind on this thread.
>
> For frontswap and zram, the "put" calls are not in interrupt
> context. For cleancache, the put call IS in interrupt context.
> So if you want to use zsmalloc for zcache+cleancache, interrupt
> context is a concern. As discussed previously in a separate
> thread though, zsmalloc will take a lot of work to support the full
> needs of zcache. So, pick your poison.
Oops, correction. Cleancache puts are not in interrupt context
but do have interrupts disabled. That's quite different of
course. So Minchan's BUG_ON(in_interrupt()) should be fine for
now.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 3/4] zsmalloc: add details to zs_map_object boiler plate
> From: Minchan Kim [mailto:minc...@kernel.org]
> Subject: Re: [PATCH 3/4] zsmalloc: add details to zs_map_object boiler plate
>
> On Wed, Jul 11, 2012 at 09:15:43AM -0500, Seth Jennings wrote:
> > On 07/11/2012 02:42 AM, Minchan Kim wrote:
> > > On 07/11/2012 12:17 AM, Seth Jennings wrote:
> > >> On 07/09/2012 09:35 PM, Minchan Kim wrote:
> > >>> Maybe we need local_irq_save/restore in zs_[un]map_object path.
> > >>
> > >> I'd rather not disable interrupts since that will create
> > >> unnecessary interrupt latency for all users, even if they
> > >
> > > Agreed.
> > > Although we guide k[un]map atomic is so fast, it isn't necessary
> > > to force irq_[enable|disable]. Okay.
> > >
> > >> don't need interrupt protection. If a particular user uses
> > >> zs_map_object() in an interrupt path, it will be up to that
> > >> user to disable interrupts to ensure safety.
> > >
> > > Nope. It shouldn't do that.
> > > Any user in interrupt context can't assume that there isn't any other
> > > user using per-cpu buffer
> > > right before interrupt happens.
> > >
> > > The concern is that if such bug happens, it's very hard to find a bug.
> > > So, how about adding this?
> > >
> > > void zs_map_object(...)
> > > {
> > > BUG_ON(in_interrupt());
> > > }
> >
> > I not completely following you, but I think I'm following
> > enough. Your point is that the per-cpu buffers are shared
> > by all zsmalloc users and one user doesn't know if another
> > user is doing a zs_map_object() in an interrupt path.
>
> And vise versa is yes.
>
> > However, I think what you are suggesting is to disallow
> > mapping in interrupt context. This is a problem for zcache
> > as it already does mapping in interrupt context, namely for
> > page decompression in the page fault handler.
>
> I don't get it.
> Page fault handler isn't interrupt context.
>
> > What do you think about making the per-cpu buffers local to
> > each zsmalloc pool? That way each user has their own per-cpu
> > buffers and don't step on each other's toes.
>
> Maybe, It could be a solution if you really need it in interrupt context.
> But the concern is it could hurt zsmalloc's goal which is memory
> space efficiency if your system has lots of CPUs.
Sorry to be so far behind on this thread.
For frontswap and zram, the "put" calls are not in interrupt
context. For cleancache, the put call IS in interrupt context.
So if you want to use zsmalloc for zcache+cleancache, interrupt
context is a concern. As discussed previously in a separate
thread though, zsmalloc will take a lot of work to support the full
needs of zcache. So, pick your poison.
The x86 architecture is far ahead of ARM on many CPU optimizations
including fast copying. Most x86 systems are also now 64-bit,
which means 64-bit+ data buses. These differences probably account
for the difference in copy-vs-TLBmapping performance between (64-bit)
x86 and (32-bit ARM). However, ARM may eventually catch up, especially
when 64-bit ARM chips are more common, so
it would be nice if zsmalloc could determine at runtime which
is faster and use it.
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 3/4] zsmalloc: add details to zs_map_object boiler plate
From: Minchan Kim [mailto:minc...@kernel.org]
Subject: Re: [PATCH 3/4] zsmalloc: add details to zs_map_object boiler plate
On Wed, Jul 11, 2012 at 09:15:43AM -0500, Seth Jennings wrote:
On 07/11/2012 02:42 AM, Minchan Kim wrote:
On 07/11/2012 12:17 AM, Seth Jennings wrote:
On 07/09/2012 09:35 PM, Minchan Kim wrote:
Maybe we need local_irq_save/restore in zs_[un]map_object path.
I'd rather not disable interrupts since that will create
unnecessary interrupt latency for all users, even if they
Agreed.
Although we guide k[un]map atomic is so fast, it isn't necessary
to force irq_[enable|disable]. Okay.
don't need interrupt protection. If a particular user uses
zs_map_object() in an interrupt path, it will be up to that
user to disable interrupts to ensure safety.
Nope. It shouldn't do that.
Any user in interrupt context can't assume that there isn't any other
user using per-cpu buffer
right before interrupt happens.
The concern is that if such bug happens, it's very hard to find a bug.
So, how about adding this?
void zs_map_object(...)
{
BUG_ON(in_interrupt());
}
I not completely following you, but I think I'm following
enough. Your point is that the per-cpu buffers are shared
by all zsmalloc users and one user doesn't know if another
user is doing a zs_map_object() in an interrupt path.
And vise versa is yes.
However, I think what you are suggesting is to disallow
mapping in interrupt context. This is a problem for zcache
as it already does mapping in interrupt context, namely for
page decompression in the page fault handler.
I don't get it.
Page fault handler isn't interrupt context.
What do you think about making the per-cpu buffers local to
each zsmalloc pool? That way each user has their own per-cpu
buffers and don't step on each other's toes.
Maybe, It could be a solution if you really need it in interrupt context.
But the concern is it could hurt zsmalloc's goal which is memory
space efficiency if your system has lots of CPUs.
Sorry to be so far behind on this thread.
For frontswap and zram, the put calls are not in interrupt
context. For cleancache, the put call IS in interrupt context.
So if you want to use zsmalloc for zcache+cleancache, interrupt
context is a concern. As discussed previously in a separate
thread though, zsmalloc will take a lot of work to support the full
needs of zcache. So, pick your poison.
The x86 architecture is far ahead of ARM on many CPU optimizations
including fast copying. Most x86 systems are also now 64-bit,
which means 64-bit+ data buses. These differences probably account
for the difference in copy-vs-TLBmapping performance between (64-bit)
x86 and (32-bit ARM). However, ARM may eventually catch up, especially
when 64-bit ARM chips are more common, so
it would be nice if zsmalloc could determine at runtime which
is faster and use it.
Dan
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 3/4] zsmalloc: add details to zs_map_object boiler plate
From: Dan Magenheimer
Subject: RE: [PATCH 3/4] zsmalloc: add details to zs_map_object boiler plate
From: Minchan Kim [mailto:minc...@kernel.org]
Subject: Re: [PATCH 3/4] zsmalloc: add details to zs_map_object boiler plate
On Wed, Jul 11, 2012 at 09:15:43AM -0500, Seth Jennings wrote:
On 07/11/2012 02:42 AM, Minchan Kim wrote:
On 07/11/2012 12:17 AM, Seth Jennings wrote:
On 07/09/2012 09:35 PM, Minchan Kim wrote:
Maybe we need local_irq_save/restore in zs_[un]map_object path.
I'd rather not disable interrupts since that will create
unnecessary interrupt latency for all users, even if they
Agreed.
Although we guide k[un]map atomic is so fast, it isn't necessary
to force irq_[enable|disable]. Okay.
don't need interrupt protection. If a particular user uses
zs_map_object() in an interrupt path, it will be up to that
user to disable interrupts to ensure safety.
Nope. It shouldn't do that.
Any user in interrupt context can't assume that there isn't any other
user using per-cpu buffer
right before interrupt happens.
The concern is that if such bug happens, it's very hard to find a bug.
So, how about adding this?
void zs_map_object(...)
{
BUG_ON(in_interrupt());
}
I not completely following you, but I think I'm following
enough. Your point is that the per-cpu buffers are shared
by all zsmalloc users and one user doesn't know if another
user is doing a zs_map_object() in an interrupt path.
And vise versa is yes.
However, I think what you are suggesting is to disallow
mapping in interrupt context. This is a problem for zcache
as it already does mapping in interrupt context, namely for
page decompression in the page fault handler.
I don't get it.
Page fault handler isn't interrupt context.
What do you think about making the per-cpu buffers local to
each zsmalloc pool? That way each user has their own per-cpu
buffers and don't step on each other's toes.
Maybe, It could be a solution if you really need it in interrupt context.
But the concern is it could hurt zsmalloc's goal which is memory
space efficiency if your system has lots of CPUs.
Sorry to be so far behind on this thread.
For frontswap and zram, the put calls are not in interrupt
context. For cleancache, the put call IS in interrupt context.
So if you want to use zsmalloc for zcache+cleancache, interrupt
context is a concern. As discussed previously in a separate
thread though, zsmalloc will take a lot of work to support the full
needs of zcache. So, pick your poison.
Oops, correction. Cleancache puts are not in interrupt context
but do have interrupts disabled. That's quite different of
course. So Minchan's BUG_ON(in_interrupt()) should be fine for
now.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] zsmalloc: add details to zs_map_object boiler plate
On Wed, Jul 11, 2012 at 09:15:43AM -0500, Seth Jennings wrote:
> On 07/11/2012 02:42 AM, Minchan Kim wrote:
> > On 07/11/2012 12:17 AM, Seth Jennings wrote:
> >> On 07/09/2012 09:35 PM, Minchan Kim wrote:
> >>> Maybe we need local_irq_save/restore in zs_[un]map_object path.
> >>
> >> I'd rather not disable interrupts since that will create
> >> unnecessary interrupt latency for all users, even if they
> >
> > Agreed.
> > Although we guide k[un]map atomic is so fast, it isn't necessary
> > to force irq_[enable|disable]. Okay.
> >
> >> don't need interrupt protection. If a particular user uses
> >> zs_map_object() in an interrupt path, it will be up to that
> >> user to disable interrupts to ensure safety.
> >
> > Nope. It shouldn't do that.
> > Any user in interrupt context can't assume that there isn't any other user
> > using per-cpu buffer
> > right before interrupt happens.
> >
> > The concern is that if such bug happens, it's very hard to find a bug.
> > So, how about adding this?
> >
> > void zs_map_object(...)
> > {
> > BUG_ON(in_interrupt());
> > }
>
> I not completely following you, but I think I'm following
> enough. Your point is that the per-cpu buffers are shared
> by all zsmalloc users and one user doesn't know if another
> user is doing a zs_map_object() in an interrupt path.
And vise versa is yes.
>
> However, I think what you are suggesting is to disallow
> mapping in interrupt context. This is a problem for zcache
> as it already does mapping in interrupt context, namely for
> page decompression in the page fault handler.
I don't get it.
Page fault handler isn't interrupt context.
>
> What do you think about making the per-cpu buffers local to
> each zsmalloc pool? That way each user has their own per-cpu
> buffers and don't step on each other's toes.
Maybe, It could be a solution if you really need it in interrupt context.
But the concern is it could hurt zsmalloc's goal which is memory
space efficiency if your system has lots of CPUs.
>
> Thanks,
> Seth
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] zsmalloc: add details to zs_map_object boiler plate
On 07/11/2012 02:42 AM, Minchan Kim wrote:
> On 07/11/2012 12:17 AM, Seth Jennings wrote:
>> On 07/09/2012 09:35 PM, Minchan Kim wrote:
>>> Maybe we need local_irq_save/restore in zs_[un]map_object path.
>>
>> I'd rather not disable interrupts since that will create
>> unnecessary interrupt latency for all users, even if they
>
> Agreed.
> Although we guide k[un]map atomic is so fast, it isn't necessary
> to force irq_[enable|disable]. Okay.
>
>> don't need interrupt protection. If a particular user uses
>> zs_map_object() in an interrupt path, it will be up to that
>> user to disable interrupts to ensure safety.
>
> Nope. It shouldn't do that.
> Any user in interrupt context can't assume that there isn't any other user
> using per-cpu buffer
> right before interrupt happens.
>
> The concern is that if such bug happens, it's very hard to find a bug.
> So, how about adding this?
>
> void zs_map_object(...)
> {
> BUG_ON(in_interrupt());
> }
I not completely following you, but I think I'm following
enough. Your point is that the per-cpu buffers are shared
by all zsmalloc users and one user doesn't know if another
user is doing a zs_map_object() in an interrupt path.
However, I think what you are suggesting is to disallow
mapping in interrupt context. This is a problem for zcache
as it already does mapping in interrupt context, namely for
page decompression in the page fault handler.
What do you think about making the per-cpu buffers local to
each zsmalloc pool? That way each user has their own per-cpu
buffers and don't step on each other's toes.
Thanks,
Seth
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] zsmalloc: add details to zs_map_object boiler plate
On 07/11/2012 12:17 AM, Seth Jennings wrote:
> On 07/09/2012 09:35 PM, Minchan Kim wrote:
>> On 07/03/2012 06:15 AM, Seth Jennings wrote:
>>> Add information on the usage limits of zs_map_object()
>>>
>>> Signed-off-by: Seth Jennings
>>> ---
>>> drivers/staging/zsmalloc/zsmalloc-main.c |7 ++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c
>>> b/drivers/staging/zsmalloc/zsmalloc-main.c
>>> index 4942d41..abf7c13 100644
>>> --- a/drivers/staging/zsmalloc/zsmalloc-main.c
>>> +++ b/drivers/staging/zsmalloc/zsmalloc-main.c
>>> @@ -747,7 +747,12 @@ EXPORT_SYMBOL_GPL(zs_free);
>>> *
>>> * Before using an object allocated from zs_malloc, it must be mapped using
>>> * this function. When done with the object, it must be unmapped using
>>> - * zs_unmap_object
>>> + * zs_unmap_object.
>>> + *
>>> + * Only one object can be mapped per cpu at a time. There is no protection
>>> + * against nested mappings.
>>> + *
>>> + * This function returns with preemption and page faults disabled.
>>> */
>>> void *zs_map_object(struct zs_pool *pool, unsigned long handle)
>>> {
>>>
>>
>> The comment is good but I hope we can detect it automatically with DEBUG
>> option. It wouldn't be hard but it's a debug patch so not critical
>> until we receive some report about the bug.
>
> Yes, we could implement some detection scheme later.
>
>>
>> The possibility for nesting is that it is used by irq context.
>>
>> A uses the mapping
>> .
>> .
>> .
>> IRQ happen
>> B uses the mapping in IRQ context
>> .
>> .
>> .
>>
>> Maybe we need local_irq_save/restore in zs_[un]map_object path.
>
> I'd rather not disable interrupts since that will create
> unnecessary interrupt latency for all users, even if they
Agreed.
Although we guide k[un]map atomic is so fast, it isn't necessary
to force irq_[enable|disable]. Okay.
> don't need interrupt protection. If a particular user uses
> zs_map_object() in an interrupt path, it will be up to that
> user to disable interrupts to ensure safety.
Nope. It shouldn't do that.
Any user in interrupt context can't assume that there isn't any other user
using per-cpu buffer
right before interrupt happens.
The concern is that if such bug happens, it's very hard to find a bug.
So, how about adding this?
void zs_map_object(...)
{
BUG_ON(in_interrupt());
}
>
> Thanks,
> Seth
>
>
--
Kind regards,
Minchan Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] zsmalloc: add details to zs_map_object boiler plate
On 07/11/2012 12:17 AM, Seth Jennings wrote:
On 07/09/2012 09:35 PM, Minchan Kim wrote:
On 07/03/2012 06:15 AM, Seth Jennings wrote:
Add information on the usage limits of zs_map_object()
Signed-off-by: Seth Jennings sjenn...@linux.vnet.ibm.com
---
drivers/staging/zsmalloc/zsmalloc-main.c |7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c
b/drivers/staging/zsmalloc/zsmalloc-main.c
index 4942d41..abf7c13 100644
--- a/drivers/staging/zsmalloc/zsmalloc-main.c
+++ b/drivers/staging/zsmalloc/zsmalloc-main.c
@@ -747,7 +747,12 @@ EXPORT_SYMBOL_GPL(zs_free);
*
* Before using an object allocated from zs_malloc, it must be mapped using
* this function. When done with the object, it must be unmapped using
- * zs_unmap_object
+ * zs_unmap_object.
+ *
+ * Only one object can be mapped per cpu at a time. There is no protection
+ * against nested mappings.
+ *
+ * This function returns with preemption and page faults disabled.
*/
void *zs_map_object(struct zs_pool *pool, unsigned long handle)
{
The comment is good but I hope we can detect it automatically with DEBUG
option. It wouldn't be hard but it's a debug patch so not critical
until we receive some report about the bug.
Yes, we could implement some detection scheme later.
The possibility for nesting is that it is used by irq context.
A uses the mapping
.
.
.
IRQ happen
B uses the mapping in IRQ context
.
.
.
Maybe we need local_irq_save/restore in zs_[un]map_object path.
I'd rather not disable interrupts since that will create
unnecessary interrupt latency for all users, even if they
Agreed.
Although we guide k[un]map atomic is so fast, it isn't necessary
to force irq_[enable|disable]. Okay.
don't need interrupt protection. If a particular user uses
zs_map_object() in an interrupt path, it will be up to that
user to disable interrupts to ensure safety.
Nope. It shouldn't do that.
Any user in interrupt context can't assume that there isn't any other user
using per-cpu buffer
right before interrupt happens.
The concern is that if such bug happens, it's very hard to find a bug.
So, how about adding this?
void zs_map_object(...)
{
BUG_ON(in_interrupt());
}
Thanks,
Seth
--
Kind regards,
Minchan Kim
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] zsmalloc: add details to zs_map_object boiler plate
On 07/11/2012 02:42 AM, Minchan Kim wrote:
On 07/11/2012 12:17 AM, Seth Jennings wrote:
On 07/09/2012 09:35 PM, Minchan Kim wrote:
Maybe we need local_irq_save/restore in zs_[un]map_object path.
I'd rather not disable interrupts since that will create
unnecessary interrupt latency for all users, even if they
Agreed.
Although we guide k[un]map atomic is so fast, it isn't necessary
to force irq_[enable|disable]. Okay.
don't need interrupt protection. If a particular user uses
zs_map_object() in an interrupt path, it will be up to that
user to disable interrupts to ensure safety.
Nope. It shouldn't do that.
Any user in interrupt context can't assume that there isn't any other user
using per-cpu buffer
right before interrupt happens.
The concern is that if such bug happens, it's very hard to find a bug.
So, how about adding this?
void zs_map_object(...)
{
BUG_ON(in_interrupt());
}
I not completely following you, but I think I'm following
enough. Your point is that the per-cpu buffers are shared
by all zsmalloc users and one user doesn't know if another
user is doing a zs_map_object() in an interrupt path.
However, I think what you are suggesting is to disallow
mapping in interrupt context. This is a problem for zcache
as it already does mapping in interrupt context, namely for
page decompression in the page fault handler.
What do you think about making the per-cpu buffers local to
each zsmalloc pool? That way each user has their own per-cpu
buffers and don't step on each other's toes.
Thanks,
Seth
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] zsmalloc: add details to zs_map_object boiler plate
On Wed, Jul 11, 2012 at 09:15:43AM -0500, Seth Jennings wrote:
On 07/11/2012 02:42 AM, Minchan Kim wrote:
On 07/11/2012 12:17 AM, Seth Jennings wrote:
On 07/09/2012 09:35 PM, Minchan Kim wrote:
Maybe we need local_irq_save/restore in zs_[un]map_object path.
I'd rather not disable interrupts since that will create
unnecessary interrupt latency for all users, even if they
Agreed.
Although we guide k[un]map atomic is so fast, it isn't necessary
to force irq_[enable|disable]. Okay.
don't need interrupt protection. If a particular user uses
zs_map_object() in an interrupt path, it will be up to that
user to disable interrupts to ensure safety.
Nope. It shouldn't do that.
Any user in interrupt context can't assume that there isn't any other user
using per-cpu buffer
right before interrupt happens.
The concern is that if such bug happens, it's very hard to find a bug.
So, how about adding this?
void zs_map_object(...)
{
BUG_ON(in_interrupt());
}
I not completely following you, but I think I'm following
enough. Your point is that the per-cpu buffers are shared
by all zsmalloc users and one user doesn't know if another
user is doing a zs_map_object() in an interrupt path.
And vise versa is yes.
However, I think what you are suggesting is to disallow
mapping in interrupt context. This is a problem for zcache
as it already does mapping in interrupt context, namely for
page decompression in the page fault handler.
I don't get it.
Page fault handler isn't interrupt context.
What do you think about making the per-cpu buffers local to
each zsmalloc pool? That way each user has their own per-cpu
buffers and don't step on each other's toes.
Maybe, It could be a solution if you really need it in interrupt context.
But the concern is it could hurt zsmalloc's goal which is memory
space efficiency if your system has lots of CPUs.
Thanks,
Seth
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] zsmalloc: add details to zs_map_object boiler plate
On 07/09/2012 09:35 PM, Minchan Kim wrote:
> On 07/03/2012 06:15 AM, Seth Jennings wrote:
>> Add information on the usage limits of zs_map_object()
>>
>> Signed-off-by: Seth Jennings
>> ---
>> drivers/staging/zsmalloc/zsmalloc-main.c |7 ++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c
>> b/drivers/staging/zsmalloc/zsmalloc-main.c
>> index 4942d41..abf7c13 100644
>> --- a/drivers/staging/zsmalloc/zsmalloc-main.c
>> +++ b/drivers/staging/zsmalloc/zsmalloc-main.c
>> @@ -747,7 +747,12 @@ EXPORT_SYMBOL_GPL(zs_free);
>> *
>> * Before using an object allocated from zs_malloc, it must be mapped using
>> * this function. When done with the object, it must be unmapped using
>> - * zs_unmap_object
>> + * zs_unmap_object.
>> + *
>> + * Only one object can be mapped per cpu at a time. There is no protection
>> + * against nested mappings.
>> + *
>> + * This function returns with preemption and page faults disabled.
>> */
>> void *zs_map_object(struct zs_pool *pool, unsigned long handle)
>> {
>>
>
> The comment is good but I hope we can detect it automatically with DEBUG
> option. It wouldn't be hard but it's a debug patch so not critical
> until we receive some report about the bug.
Yes, we could implement some detection scheme later.
>
> The possibility for nesting is that it is used by irq context.
>
> A uses the mapping
> .
> .
> .
> IRQ happen
> B uses the mapping in IRQ context
> .
> .
> .
>
> Maybe we need local_irq_save/restore in zs_[un]map_object path.
I'd rather not disable interrupts since that will create
unnecessary interrupt latency for all users, even if they
don't need interrupt protection. If a particular user uses
zs_map_object() in an interrupt path, it will be up to that
user to disable interrupts to ensure safety.
Thanks,
Seth
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] zsmalloc: add details to zs_map_object boiler plate
On 07/09/2012 09:35 PM, Minchan Kim wrote:
On 07/03/2012 06:15 AM, Seth Jennings wrote:
Add information on the usage limits of zs_map_object()
Signed-off-by: Seth Jennings sjenn...@linux.vnet.ibm.com
---
drivers/staging/zsmalloc/zsmalloc-main.c |7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c
b/drivers/staging/zsmalloc/zsmalloc-main.c
index 4942d41..abf7c13 100644
--- a/drivers/staging/zsmalloc/zsmalloc-main.c
+++ b/drivers/staging/zsmalloc/zsmalloc-main.c
@@ -747,7 +747,12 @@ EXPORT_SYMBOL_GPL(zs_free);
*
* Before using an object allocated from zs_malloc, it must be mapped using
* this function. When done with the object, it must be unmapped using
- * zs_unmap_object
+ * zs_unmap_object.
+ *
+ * Only one object can be mapped per cpu at a time. There is no protection
+ * against nested mappings.
+ *
+ * This function returns with preemption and page faults disabled.
*/
void *zs_map_object(struct zs_pool *pool, unsigned long handle)
{
The comment is good but I hope we can detect it automatically with DEBUG
option. It wouldn't be hard but it's a debug patch so not critical
until we receive some report about the bug.
Yes, we could implement some detection scheme later.
The possibility for nesting is that it is used by irq context.
A uses the mapping
.
.
.
IRQ happen
B uses the mapping in IRQ context
.
.
.
Maybe we need local_irq_save/restore in zs_[un]map_object path.
I'd rather not disable interrupts since that will create
unnecessary interrupt latency for all users, even if they
don't need interrupt protection. If a particular user uses
zs_map_object() in an interrupt path, it will be up to that
user to disable interrupts to ensure safety.
Thanks,
Seth
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] zsmalloc: add details to zs_map_object boiler plate
On 07/03/2012 06:15 AM, Seth Jennings wrote:
> Add information on the usage limits of zs_map_object()
>
> Signed-off-by: Seth Jennings
> ---
> drivers/staging/zsmalloc/zsmalloc-main.c |7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c
> b/drivers/staging/zsmalloc/zsmalloc-main.c
> index 4942d41..abf7c13 100644
> --- a/drivers/staging/zsmalloc/zsmalloc-main.c
> +++ b/drivers/staging/zsmalloc/zsmalloc-main.c
> @@ -747,7 +747,12 @@ EXPORT_SYMBOL_GPL(zs_free);
> *
> * Before using an object allocated from zs_malloc, it must be mapped using
> * this function. When done with the object, it must be unmapped using
> - * zs_unmap_object
> + * zs_unmap_object.
> + *
> + * Only one object can be mapped per cpu at a time. There is no protection
> + * against nested mappings.
> + *
> + * This function returns with preemption and page faults disabled.
> */
> void *zs_map_object(struct zs_pool *pool, unsigned long handle)
> {
>
The comment is good but I hope we can detect it automatically with DEBUG
option. It wouldn't be hard but it's a debug patch so not critical
until we receive some report about the bug.
The possibility for nesting is that it is used by irq context.
A uses the mapping
.
.
.
IRQ happen
B uses the mapping in IRQ context
.
.
.
Maybe we need local_irq_save/restore in zs_[un]map_object path.
--
Kind regards,
Minchan Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] zsmalloc: add details to zs_map_object boiler plate
On 07/03/2012 06:15 AM, Seth Jennings wrote:
Add information on the usage limits of zs_map_object()
Signed-off-by: Seth Jennings sjenn...@linux.vnet.ibm.com
---
drivers/staging/zsmalloc/zsmalloc-main.c |7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c
b/drivers/staging/zsmalloc/zsmalloc-main.c
index 4942d41..abf7c13 100644
--- a/drivers/staging/zsmalloc/zsmalloc-main.c
+++ b/drivers/staging/zsmalloc/zsmalloc-main.c
@@ -747,7 +747,12 @@ EXPORT_SYMBOL_GPL(zs_free);
*
* Before using an object allocated from zs_malloc, it must be mapped using
* this function. When done with the object, it must be unmapped using
- * zs_unmap_object
+ * zs_unmap_object.
+ *
+ * Only one object can be mapped per cpu at a time. There is no protection
+ * against nested mappings.
+ *
+ * This function returns with preemption and page faults disabled.
*/
void *zs_map_object(struct zs_pool *pool, unsigned long handle)
{
The comment is good but I hope we can detect it automatically with DEBUG
option. It wouldn't be hard but it's a debug patch so not critical
until we receive some report about the bug.
The possibility for nesting is that it is used by irq context.
A uses the mapping
.
.
.
IRQ happen
B uses the mapping in IRQ context
.
.
.
Maybe we need local_irq_save/restore in zs_[un]map_object path.
--
Kind regards,
Minchan Kim
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
