Re: [PATCH 0/2] memcgroup: work better with tmpfs
Hugh Dickins wrote:
> On Wed, 19 Dec 2007, Balbir Singh wrote:
>> Hugh Dickins wrote:
>>
>> We always call mem_cgroup_isolate_pages() from shrink_(in)active_pages
>> under spin_lock_irq of the zone's lru lock. That's the reason that we
>> don't explicitly use it in the routine.
>
> Indeed, thanks.
>
>>> 2. There's mem_cgroup_charge and mem_cgroup_cache_charge (wouldn't the
>>> former be better called mem_cgroup_charge_mapped? why does the latter
>> Yes, it would be. After we've refactored the code, the new name makes sense.
>>
>>> test MEM_CGROUP_TYPE_ALL instead of MEM_CGROUP_TYPE_CACHED? I still don't
>>> understand your enums there).
>> We do that to ensure that we charge page cache pages only when the
>> accounting type is set to MEM_CGROUP_TYPE_ALL. If the type is anything
>> else, we ignore cached pages, we did not have MEM_CGROUP_TYPE_CACHED
>> initially when the patches went in.
>
> I think you've given yourself far too many degrees of freedom here.
>
> Please explain to me how the system is supposed to behave when I
> echo -n 2 >/cg/0/memory.control_type
>
> From the name in the enum (MEM_CGROUP_TYPE_CACHED, doesn't even have
> an "= 2", yet this is a part of the API?), I think it's supposed to
> account pages into and out of the page cache, but not as they're
> mapped into and out of userspace. But from the code, no references
> whatever to MEM_CGROUP_TYPE_CACHED or MEM_CGROUP_TYPE_MAPPED,
> it looks as if it'll behave just like MEM_CGROUP_TYPE_MAPPED
> (which we hope = 1).
>
> As you say, you didn't have MEM_CGROUP_TYPE_CACHED originally:
> it looks like it got added to the straightforward case of MAPPED,
> in an apparently flexible but not fully thought out way.
>
Yes, I think your argument makes sense. I had initially thought of
making the enums a mask
MEM_CGROUP_TYPE_MAPPED = 0x1,
MEM_CGROUP_TYPE_CACHED = 0x2,
MEM_CGROUP_TYPE_ALL = 0x3
and check for control_type & MEM_CGROUP_TYPE_CACHED
>> But there's only mem_cgroup_uncharge.
>>> So when, for example, an add_to_page_cache fails, the uncharge may not
>>> balance the charge?
>>>
>> We use mem_cgroup_uncharge() everywhere. The reason being, we might
>> switch control type, we uncharge pages that have a page_cgroup
>> associated with them, hence once we;ve charged, uncharge does not
>> distinguish between charge types.
>
> Ah, so this is the meaning of that
> /*
>* This can handle cases when a page is not charged at all and we
>* are switching between handling the control_type.
>*/
> if (!pc)
> return;
>
> if (atomic_dec_and_test(>ref_cnt)) {
> at the beginning of mem_cgroup_uncharge.
>
> Sorry, that doesn't fly. You've no locking between acquiring pc from
> the page->page_cgroup, testing pc here, and decrementing its ref_cnt.
> When that ref_cnt goes down to zero, clear_page_cgroup may NULLify
> page->page_cgroup and kfree the pc.
>
Yes, we need to lock the page group.
> So if you cannot really keep track of the ref_cnt (because of
> changing control_type), that atomic_dec_and_test is in danger
> of corrupting someone else's memory - isn't it?
>
Yes, my bad :(
> I can just about imagine that some admins will want control_type
> MAPPED and others CACHED and others MAPPED+CACHED. But is there
> actually a need for one cgroup to be controlling MAPPED while
> another on the same machine is controlling MAPPED+CACHED? And
> does that make sense - there'll be weirdnesses, won't there?
> And is there actually a need to change a cgroup's control_type
> on the fly while it's alive?
>
> Of course it's nice to be flexible and allow for such possibilities;
> but not if that just opens windows for corruption. My own view is
> that at present you should just account mapped and cached for all,
> and strip out these extra degrees of freedom: add them back in some
> future when the locking is worked out.
>
I am going to rip out the control_type interface for now. The locking
needs fixing irrespective of control_type. I am sending out a patch to
rip out control_type.
> Hugh
Thanks for your detailed comments,
--
Warm Regards,
Balbir Singh
Linux Technology Center
IBM, ISTL
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] memcgroup: work better with tmpfs
On Wed, 19 Dec 2007, Balbir Singh wrote:
> Hugh Dickins wrote:
>
> We always call mem_cgroup_isolate_pages() from shrink_(in)active_pages
> under spin_lock_irq of the zone's lru lock. That's the reason that we
> don't explicitly use it in the routine.
Indeed, thanks.
>
> > 2. There's mem_cgroup_charge and mem_cgroup_cache_charge (wouldn't the
> > former be better called mem_cgroup_charge_mapped? why does the latter
>
> Yes, it would be. After we've refactored the code, the new name makes sense.
>
> > test MEM_CGROUP_TYPE_ALL instead of MEM_CGROUP_TYPE_CACHED? I still don't
> > understand your enums there).
>
> We do that to ensure that we charge page cache pages only when the
> accounting type is set to MEM_CGROUP_TYPE_ALL. If the type is anything
> else, we ignore cached pages, we did not have MEM_CGROUP_TYPE_CACHED
> initially when the patches went in.
I think you've given yourself far too many degrees of freedom here.
Please explain to me how the system is supposed to behave when I
echo -n 2 >/cg/0/memory.control_type
>From the name in the enum (MEM_CGROUP_TYPE_CACHED, doesn't even have
an "= 2", yet this is a part of the API?), I think it's supposed to
account pages into and out of the page cache, but not as they're
mapped into and out of userspace. But from the code, no references
whatever to MEM_CGROUP_TYPE_CACHED or MEM_CGROUP_TYPE_MAPPED,
it looks as if it'll behave just like MEM_CGROUP_TYPE_MAPPED
(which we hope = 1).
As you say, you didn't have MEM_CGROUP_TYPE_CACHED originally:
it looks like it got added to the straightforward case of MAPPED,
in an apparently flexible but not fully thought out way.
>
> But there's only mem_cgroup_uncharge.
> > So when, for example, an add_to_page_cache fails, the uncharge may not
> > balance the charge?
> >
>
> We use mem_cgroup_uncharge() everywhere. The reason being, we might
> switch control type, we uncharge pages that have a page_cgroup
> associated with them, hence once we;ve charged, uncharge does not
> distinguish between charge types.
Ah, so this is the meaning of that
/*
* This can handle cases when a page is not charged at all and we
* are switching between handling the control_type.
*/
if (!pc)
return;
if (atomic_dec_and_test(>ref_cnt)) {
at the beginning of mem_cgroup_uncharge.
Sorry, that doesn't fly. You've no locking between acquiring pc from
the page->page_cgroup, testing pc here, and decrementing its ref_cnt.
When that ref_cnt goes down to zero, clear_page_cgroup may NULLify
page->page_cgroup and kfree the pc.
So if you cannot really keep track of the ref_cnt (because of
changing control_type), that atomic_dec_and_test is in danger
of corrupting someone else's memory - isn't it?
I can just about imagine that some admins will want control_type
MAPPED and others CACHED and others MAPPED+CACHED. But is there
actually a need for one cgroup to be controlling MAPPED while
another on the same machine is controlling MAPPED+CACHED? And
does that make sense - there'll be weirdnesses, won't there?
And is there actually a need to change a cgroup's control_type
on the fly while it's alive?
Of course it's nice to be flexible and allow for such possibilities;
but not if that just opens windows for corruption. My own view is
that at present you should just account mapped and cached for all,
and strip out these extra degrees of freedom: add them back in some
future when the locking is worked out.
Hugh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] memcgroup: work better with tmpfs
On Wed, 19 Dec 2007, KAMEZAWA Hiroyuki wrote: > On Tue, 18 Dec 2007 22:19:22 + (GMT) > Hugh Dickins <[EMAIL PROTECTED]> wrote: > > 1. Why is spin_lock_irqsave rather than spin_lock needed on mz->lru_lock? > > If it is needed, doesn't mem_cgroup_isolate_pages need to use it too? > > > When I wrote a patch to treat lru_lock (it was not per-zone yet.), I got a > comment to use irqsafe version. So I wonder there is some plan which needs > irq safe locking. It might have come about because struct zone's lru_lock really does need the irq disabled (because of things that happen at I/O completion time). But I don't think struct mem_cgroup_per_zone's lru_lock needs it. > About mem_cgroup_isolate_pages(), zone->lock is acquired with irq-disable > before it is called. Then, it's not necesary there. Ah, yes, I completely missed that, thank you. > > But as you say, it looks we can do it without irq-disable, now. > > > > 2. There's mem_cgroup_charge and mem_cgroup_cache_charge (wouldn't the > > former be better called mem_cgroup_charge_mapped? why does the latter > > test MEM_CGROUP_TYPE_ALL instead of MEM_CGROUP_TYPE_CACHED? I still don't > > understand your enums there). But there's only mem_cgroup_uncharge. > > So when, for example, an add_to_page_cache fails, the uncharge may not > > balance the charge? > > > Ah...it seems bug. We should add type handling in uncharge. > Then, changing control_type after start using mem_cgroup seems dangerous. > (Default is ALL now.) > > Maybe following will be fix. > > - allow changing contorl_type only when there is no task. > - run force_empty when control_type is changed. and drop all charges. > > This will change current behavior but I think it's reasonable. > How do you think ? Perhaps... my grasp on this is much too weak to be sure: I'm pretty sceptical about changing control_type altogether. Let me reply to Balbir, who still sees no issue there. Hugh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] memcgroup: work better with tmpfs
On Wed, 19 Dec 2007, KAMEZAWA Hiroyuki wrote: On Tue, 18 Dec 2007 22:19:22 + (GMT) Hugh Dickins [EMAIL PROTECTED] wrote: 1. Why is spin_lock_irqsave rather than spin_lock needed on mz-lru_lock? If it is needed, doesn't mem_cgroup_isolate_pages need to use it too? When I wrote a patch to treat lru_lock (it was not per-zone yet.), I got a comment to use irqsafe version. So I wonder there is some plan which needs irq safe locking. It might have come about because struct zone's lru_lock really does need the irq disabled (because of things that happen at I/O completion time). But I don't think struct mem_cgroup_per_zone's lru_lock needs it. About mem_cgroup_isolate_pages(), zone-lock is acquired with irq-disable before it is called. Then, it's not necesary there. Ah, yes, I completely missed that, thank you. But as you say, it looks we can do it without irq-disable, now. 2. There's mem_cgroup_charge and mem_cgroup_cache_charge (wouldn't the former be better called mem_cgroup_charge_mapped? why does the latter test MEM_CGROUP_TYPE_ALL instead of MEM_CGROUP_TYPE_CACHED? I still don't understand your enums there). But there's only mem_cgroup_uncharge. So when, for example, an add_to_page_cache fails, the uncharge may not balance the charge? Ah...it seems bug. We should add type handling in uncharge. Then, changing control_type after start using mem_cgroup seems dangerous. (Default is ALL now.) Maybe following will be fix. - allow changing contorl_type only when there is no task. - run force_empty when control_type is changed. and drop all charges. This will change current behavior but I think it's reasonable. How do you think ? Perhaps... my grasp on this is much too weak to be sure: I'm pretty sceptical about changing control_type altogether. Let me reply to Balbir, who still sees no issue there. Hugh -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] memcgroup: work better with tmpfs
On Wed, 19 Dec 2007, Balbir Singh wrote:
Hugh Dickins wrote:
We always call mem_cgroup_isolate_pages() from shrink_(in)active_pages
under spin_lock_irq of the zone's lru lock. That's the reason that we
don't explicitly use it in the routine.
Indeed, thanks.
2. There's mem_cgroup_charge and mem_cgroup_cache_charge (wouldn't the
former be better called mem_cgroup_charge_mapped? why does the latter
Yes, it would be. After we've refactored the code, the new name makes sense.
test MEM_CGROUP_TYPE_ALL instead of MEM_CGROUP_TYPE_CACHED? I still don't
understand your enums there).
We do that to ensure that we charge page cache pages only when the
accounting type is set to MEM_CGROUP_TYPE_ALL. If the type is anything
else, we ignore cached pages, we did not have MEM_CGROUP_TYPE_CACHED
initially when the patches went in.
I think you've given yourself far too many degrees of freedom here.
Please explain to me how the system is supposed to behave when I
echo -n 2 /cg/0/memory.control_type
From the name in the enum (MEM_CGROUP_TYPE_CACHED, doesn't even have
an = 2, yet this is a part of the API?), I think it's supposed to
account pages into and out of the page cache, but not as they're
mapped into and out of userspace. But from the code, no references
whatever to MEM_CGROUP_TYPE_CACHED or MEM_CGROUP_TYPE_MAPPED,
it looks as if it'll behave just like MEM_CGROUP_TYPE_MAPPED
(which we hope = 1).
As you say, you didn't have MEM_CGROUP_TYPE_CACHED originally:
it looks like it got added to the straightforward case of MAPPED,
in an apparently flexible but not fully thought out way.
But there's only mem_cgroup_uncharge.
So when, for example, an add_to_page_cache fails, the uncharge may not
balance the charge?
We use mem_cgroup_uncharge() everywhere. The reason being, we might
switch control type, we uncharge pages that have a page_cgroup
associated with them, hence once we;ve charged, uncharge does not
distinguish between charge types.
Ah, so this is the meaning of that
/*
* This can handle cases when a page is not charged at all and we
* are switching between handling the control_type.
*/
if (!pc)
return;
if (atomic_dec_and_test(pc-ref_cnt)) {
at the beginning of mem_cgroup_uncharge.
Sorry, that doesn't fly. You've no locking between acquiring pc from
the page-page_cgroup, testing pc here, and decrementing its ref_cnt.
When that ref_cnt goes down to zero, clear_page_cgroup may NULLify
page-page_cgroup and kfree the pc.
So if you cannot really keep track of the ref_cnt (because of
changing control_type), that atomic_dec_and_test is in danger
of corrupting someone else's memory - isn't it?
I can just about imagine that some admins will want control_type
MAPPED and others CACHED and others MAPPED+CACHED. But is there
actually a need for one cgroup to be controlling MAPPED while
another on the same machine is controlling MAPPED+CACHED? And
does that make sense - there'll be weirdnesses, won't there?
And is there actually a need to change a cgroup's control_type
on the fly while it's alive?
Of course it's nice to be flexible and allow for such possibilities;
but not if that just opens windows for corruption. My own view is
that at present you should just account mapped and cached for all,
and strip out these extra degrees of freedom: add them back in some
future when the locking is worked out.
Hugh
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] memcgroup: work better with tmpfs
Hugh Dickins wrote:
On Wed, 19 Dec 2007, Balbir Singh wrote:
Hugh Dickins wrote:
We always call mem_cgroup_isolate_pages() from shrink_(in)active_pages
under spin_lock_irq of the zone's lru lock. That's the reason that we
don't explicitly use it in the routine.
Indeed, thanks.
2. There's mem_cgroup_charge and mem_cgroup_cache_charge (wouldn't the
former be better called mem_cgroup_charge_mapped? why does the latter
Yes, it would be. After we've refactored the code, the new name makes sense.
test MEM_CGROUP_TYPE_ALL instead of MEM_CGROUP_TYPE_CACHED? I still don't
understand your enums there).
We do that to ensure that we charge page cache pages only when the
accounting type is set to MEM_CGROUP_TYPE_ALL. If the type is anything
else, we ignore cached pages, we did not have MEM_CGROUP_TYPE_CACHED
initially when the patches went in.
I think you've given yourself far too many degrees of freedom here.
Please explain to me how the system is supposed to behave when I
echo -n 2 /cg/0/memory.control_type
From the name in the enum (MEM_CGROUP_TYPE_CACHED, doesn't even have
an = 2, yet this is a part of the API?), I think it's supposed to
account pages into and out of the page cache, but not as they're
mapped into and out of userspace. But from the code, no references
whatever to MEM_CGROUP_TYPE_CACHED or MEM_CGROUP_TYPE_MAPPED,
it looks as if it'll behave just like MEM_CGROUP_TYPE_MAPPED
(which we hope = 1).
As you say, you didn't have MEM_CGROUP_TYPE_CACHED originally:
it looks like it got added to the straightforward case of MAPPED,
in an apparently flexible but not fully thought out way.
Yes, I think your argument makes sense. I had initially thought of
making the enums a mask
MEM_CGROUP_TYPE_MAPPED = 0x1,
MEM_CGROUP_TYPE_CACHED = 0x2,
MEM_CGROUP_TYPE_ALL = 0x3
and check for control_type MEM_CGROUP_TYPE_CACHED
But there's only mem_cgroup_uncharge.
So when, for example, an add_to_page_cache fails, the uncharge may not
balance the charge?
We use mem_cgroup_uncharge() everywhere. The reason being, we might
switch control type, we uncharge pages that have a page_cgroup
associated with them, hence once we;ve charged, uncharge does not
distinguish between charge types.
Ah, so this is the meaning of that
/*
* This can handle cases when a page is not charged at all and we
* are switching between handling the control_type.
*/
if (!pc)
return;
if (atomic_dec_and_test(pc-ref_cnt)) {
at the beginning of mem_cgroup_uncharge.
Sorry, that doesn't fly. You've no locking between acquiring pc from
the page-page_cgroup, testing pc here, and decrementing its ref_cnt.
When that ref_cnt goes down to zero, clear_page_cgroup may NULLify
page-page_cgroup and kfree the pc.
Yes, we need to lock the page group.
So if you cannot really keep track of the ref_cnt (because of
changing control_type), that atomic_dec_and_test is in danger
of corrupting someone else's memory - isn't it?
Yes, my bad :(
I can just about imagine that some admins will want control_type
MAPPED and others CACHED and others MAPPED+CACHED. But is there
actually a need for one cgroup to be controlling MAPPED while
another on the same machine is controlling MAPPED+CACHED? And
does that make sense - there'll be weirdnesses, won't there?
And is there actually a need to change a cgroup's control_type
on the fly while it's alive?
Of course it's nice to be flexible and allow for such possibilities;
but not if that just opens windows for corruption. My own view is
that at present you should just account mapped and cached for all,
and strip out these extra degrees of freedom: add them back in some
future when the locking is worked out.
I am going to rip out the control_type interface for now. The locking
needs fixing irrespective of control_type. I am sending out a patch to
rip out control_type.
Hugh
Thanks for your detailed comments,
--
Warm Regards,
Balbir Singh
Linux Technology Center
IBM, ISTL
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] memcgroup: work better with tmpfs
Hugh Dickins wrote: > Here's a couple of patches to get memcgroups working better with tmpfs > and shmem, in conjunction with the tmpfs patches I just posted. There > will be another to come later on, but I shouldn't wait any longer to get > these out to you. > Hi, Hugh, Thank you so much for the review, some comments below > (The missing patch will want to leave a mem_cgroup associated with a tmpfs > file or shm object, so that if its pages get brought back from swap by > swapoff, they can be associated with that mem_cgroup rather than the one > which happens to be running swapoff.) > > mm/memcontrol.c | 81 -- > mm/shmem.c | 28 +++ > 2 files changed, 63 insertions(+), 46 deletions(-) > > But on the way I've noticed a number of issues with memcgroups not dealt > with in these patches. > > 1. Why is spin_lock_irqsave rather than spin_lock needed on mz->lru_lock? > If it is needed, doesn't mem_cgroup_isolate_pages need to use it too? > We always call mem_cgroup_isolate_pages() from shrink_(in)active_pages under spin_lock_irq of the zone's lru lock. That's the reason that we don't explicitly use it in the routine. > 2. There's mem_cgroup_charge and mem_cgroup_cache_charge (wouldn't the > former be better called mem_cgroup_charge_mapped? why does the latter Yes, it would be. After we've refactored the code, the new name makes sense. > test MEM_CGROUP_TYPE_ALL instead of MEM_CGROUP_TYPE_CACHED? I still don't > understand your enums there). We do that to ensure that we charge page cache pages only when the accounting type is set to MEM_CGROUP_TYPE_ALL. If the type is anything else, we ignore cached pages, we did not have MEM_CGROUP_TYPE_CACHED initially when the patches went in. But there's only mem_cgroup_uncharge. > So when, for example, an add_to_page_cache fails, the uncharge may not > balance the charge? > We use mem_cgroup_uncharge() everywhere. The reason being, we might switch control type, we uncharge pages that have a page_cgroup associated with them, hence once we;ve charged, uncharge does not distinguish between charge types. > 3. mem_cgroup_charge_common has rcu_read_lock/unlock around its > rcu_dereference; mem_cgroup_cache_charge does not: is that right? > Very good catch! Will fix it. > 4. That page_assign_page_cgroup in free_hot_cold_page, what case is that > handling? Wouldn't there be a leak if it ever happens? I've been running > with a BUG_ON(page->page_cgroup) there and not hit it - should it perhaps > be a "Bad page state" case? > Our cleanup in page_cache_uncharge() does take care of cleaning up the page_cgroup. I think you've got it right, it should be a BUG_ON in free_hot_cold_page() > Hugh Thanks for the detailed review and fixes. -- Warm Regards, Balbir Singh Linux Technology Center IBM, ISTL -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] memcgroup: work better with tmpfs
On Tue, 18 Dec 2007 22:19:22 + (GMT) Hugh Dickins <[EMAIL PROTECTED]> wrote: > 1. Why is spin_lock_irqsave rather than spin_lock needed on mz->lru_lock? > If it is needed, doesn't mem_cgroup_isolate_pages need to use it too? > When I wrote a patch to treat lru_lock (it was not per-zone yet.), I got a comment to use irqsafe version. So I wonder there is some plan which needs irq safe locking. About mem_cgroup_isolate_pages(), zone->lock is acquired with irq-disable before it is called. Then, it's not necesary there. But as you say, it looks we can do it without irq-disable, now. > 2. There's mem_cgroup_charge and mem_cgroup_cache_charge (wouldn't the > former be better called mem_cgroup_charge_mapped? why does the latter > test MEM_CGROUP_TYPE_ALL instead of MEM_CGROUP_TYPE_CACHED? I still don't > understand your enums there). But there's only mem_cgroup_uncharge. > So when, for example, an add_to_page_cache fails, the uncharge may not > balance the charge? > Ah...it seems bug. We should add type handling in uncharge. Then, changing control_type after start using mem_cgroup seems dangerous. (Default is ALL now.) Maybe following will be fix. - allow changing contorl_type only when there is no task. - run force_empty when control_type is changed. and drop all charges. This will change current behavior but I think it's reasonable. How do you think ? > 3. mem_cgroup_charge_common has rcu_read_lock/unlock around its > rcu_dereference; mem_cgroup_cache_charge does not: is that right? > As you say, it seems bug. (sigh, my bug..) I'd like to fix. > 4. That page_assign_page_cgroup in free_hot_cold_page, what case is that > handling? Wouldn't there be a leak if it ever happens? I've been running > with a BUG_ON(page->page_cgroup) there and not hit it - should it perhaps > be a "Bad page state" case? I agree with you. Thank you for review ! Regards -Kame -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/2] memcgroup: work better with tmpfs
Here's a couple of patches to get memcgroups working better with tmpfs and shmem, in conjunction with the tmpfs patches I just posted. There will be another to come later on, but I shouldn't wait any longer to get these out to you. (The missing patch will want to leave a mem_cgroup associated with a tmpfs file or shm object, so that if its pages get brought back from swap by swapoff, they can be associated with that mem_cgroup rather than the one which happens to be running swapoff.) mm/memcontrol.c | 81 -- mm/shmem.c | 28 +++ 2 files changed, 63 insertions(+), 46 deletions(-) But on the way I've noticed a number of issues with memcgroups not dealt with in these patches. 1. Why is spin_lock_irqsave rather than spin_lock needed on mz->lru_lock? If it is needed, doesn't mem_cgroup_isolate_pages need to use it too? 2. There's mem_cgroup_charge and mem_cgroup_cache_charge (wouldn't the former be better called mem_cgroup_charge_mapped? why does the latter test MEM_CGROUP_TYPE_ALL instead of MEM_CGROUP_TYPE_CACHED? I still don't understand your enums there). But there's only mem_cgroup_uncharge. So when, for example, an add_to_page_cache fails, the uncharge may not balance the charge? 3. mem_cgroup_charge_common has rcu_read_lock/unlock around its rcu_dereference; mem_cgroup_cache_charge does not: is that right? 4. That page_assign_page_cgroup in free_hot_cold_page, what case is that handling? Wouldn't there be a leak if it ever happens? I've been running with a BUG_ON(page->page_cgroup) there and not hit it - should it perhaps be a "Bad page state" case? Hugh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/2] memcgroup: work better with tmpfs
Here's a couple of patches to get memcgroups working better with tmpfs and shmem, in conjunction with the tmpfs patches I just posted. There will be another to come later on, but I shouldn't wait any longer to get these out to you. (The missing patch will want to leave a mem_cgroup associated with a tmpfs file or shm object, so that if its pages get brought back from swap by swapoff, they can be associated with that mem_cgroup rather than the one which happens to be running swapoff.) mm/memcontrol.c | 81 -- mm/shmem.c | 28 +++ 2 files changed, 63 insertions(+), 46 deletions(-) But on the way I've noticed a number of issues with memcgroups not dealt with in these patches. 1. Why is spin_lock_irqsave rather than spin_lock needed on mz-lru_lock? If it is needed, doesn't mem_cgroup_isolate_pages need to use it too? 2. There's mem_cgroup_charge and mem_cgroup_cache_charge (wouldn't the former be better called mem_cgroup_charge_mapped? why does the latter test MEM_CGROUP_TYPE_ALL instead of MEM_CGROUP_TYPE_CACHED? I still don't understand your enums there). But there's only mem_cgroup_uncharge. So when, for example, an add_to_page_cache fails, the uncharge may not balance the charge? 3. mem_cgroup_charge_common has rcu_read_lock/unlock around its rcu_dereference; mem_cgroup_cache_charge does not: is that right? 4. That page_assign_page_cgroup in free_hot_cold_page, what case is that handling? Wouldn't there be a leak if it ever happens? I've been running with a BUG_ON(page-page_cgroup) there and not hit it - should it perhaps be a Bad page state case? Hugh -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] memcgroup: work better with tmpfs
On Tue, 18 Dec 2007 22:19:22 + (GMT) Hugh Dickins [EMAIL PROTECTED] wrote: 1. Why is spin_lock_irqsave rather than spin_lock needed on mz-lru_lock? If it is needed, doesn't mem_cgroup_isolate_pages need to use it too? When I wrote a patch to treat lru_lock (it was not per-zone yet.), I got a comment to use irqsafe version. So I wonder there is some plan which needs irq safe locking. About mem_cgroup_isolate_pages(), zone-lock is acquired with irq-disable before it is called. Then, it's not necesary there. But as you say, it looks we can do it without irq-disable, now. 2. There's mem_cgroup_charge and mem_cgroup_cache_charge (wouldn't the former be better called mem_cgroup_charge_mapped? why does the latter test MEM_CGROUP_TYPE_ALL instead of MEM_CGROUP_TYPE_CACHED? I still don't understand your enums there). But there's only mem_cgroup_uncharge. So when, for example, an add_to_page_cache fails, the uncharge may not balance the charge? Ah...it seems bug. We should add type handling in uncharge. Then, changing control_type after start using mem_cgroup seems dangerous. (Default is ALL now.) Maybe following will be fix. - allow changing contorl_type only when there is no task. - run force_empty when control_type is changed. and drop all charges. This will change current behavior but I think it's reasonable. How do you think ? 3. mem_cgroup_charge_common has rcu_read_lock/unlock around its rcu_dereference; mem_cgroup_cache_charge does not: is that right? As you say, it seems bug. (sigh, my bug..) I'd like to fix. 4. That page_assign_page_cgroup in free_hot_cold_page, what case is that handling? Wouldn't there be a leak if it ever happens? I've been running with a BUG_ON(page-page_cgroup) there and not hit it - should it perhaps be a Bad page state case? I agree with you. Thank you for review ! Regards -Kame -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] memcgroup: work better with tmpfs
Hugh Dickins wrote: Here's a couple of patches to get memcgroups working better with tmpfs and shmem, in conjunction with the tmpfs patches I just posted. There will be another to come later on, but I shouldn't wait any longer to get these out to you. Hi, Hugh, Thank you so much for the review, some comments below (The missing patch will want to leave a mem_cgroup associated with a tmpfs file or shm object, so that if its pages get brought back from swap by swapoff, they can be associated with that mem_cgroup rather than the one which happens to be running swapoff.) mm/memcontrol.c | 81 -- mm/shmem.c | 28 +++ 2 files changed, 63 insertions(+), 46 deletions(-) But on the way I've noticed a number of issues with memcgroups not dealt with in these patches. 1. Why is spin_lock_irqsave rather than spin_lock needed on mz-lru_lock? If it is needed, doesn't mem_cgroup_isolate_pages need to use it too? We always call mem_cgroup_isolate_pages() from shrink_(in)active_pages under spin_lock_irq of the zone's lru lock. That's the reason that we don't explicitly use it in the routine. 2. There's mem_cgroup_charge and mem_cgroup_cache_charge (wouldn't the former be better called mem_cgroup_charge_mapped? why does the latter Yes, it would be. After we've refactored the code, the new name makes sense. test MEM_CGROUP_TYPE_ALL instead of MEM_CGROUP_TYPE_CACHED? I still don't understand your enums there). We do that to ensure that we charge page cache pages only when the accounting type is set to MEM_CGROUP_TYPE_ALL. If the type is anything else, we ignore cached pages, we did not have MEM_CGROUP_TYPE_CACHED initially when the patches went in. But there's only mem_cgroup_uncharge. So when, for example, an add_to_page_cache fails, the uncharge may not balance the charge? We use mem_cgroup_uncharge() everywhere. The reason being, we might switch control type, we uncharge pages that have a page_cgroup associated with them, hence once we;ve charged, uncharge does not distinguish between charge types. 3. mem_cgroup_charge_common has rcu_read_lock/unlock around its rcu_dereference; mem_cgroup_cache_charge does not: is that right? Very good catch! Will fix it. 4. That page_assign_page_cgroup in free_hot_cold_page, what case is that handling? Wouldn't there be a leak if it ever happens? I've been running with a BUG_ON(page-page_cgroup) there and not hit it - should it perhaps be a Bad page state case? Our cleanup in page_cache_uncharge() does take care of cleaning up the page_cgroup. I think you've got it right, it should be a BUG_ON in free_hot_cold_page() Hugh Thanks for the detailed review and fixes. -- Warm Regards, Balbir Singh Linux Technology Center IBM, ISTL -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
