Re: [PATCH 05/12] mm: trylock_page
On Sunday 30 September 2007 01:01, Peter Zijlstra wrote: > On Fri, 2007-09-28 at 13:11 +1000, Nick Piggin wrote: > > On Friday 28 September 2007 17:42, Peter Zijlstra wrote: > > > Replace raw TestSetPageLocked() usage with trylock_page() > > > > I have such a thing queued too, for the lock bitops patches for when > > 2.6.24 opens, Andrew promises me :). > > > > I guess they should be identical, except I don't like doing trylock_page > > in place of SetPageLocked, for memory ordering performance and aesthetic > > reasons... I've got an init_page_locked (or set_page_locked... I can't > > remember, the patch is at home). > > Sure, that might work, or we could just make it so that add_to_*_cache > is never passed an unlocked page. But sure... It does kind of make sense to have it there (because you want the page to be locked iff it gets inserted into pagecache). And wherever you lock the page, we'll still want an init_page_locked to be able to lock it while we are the only owner of it, for the same performance reason. > > Fine idea to lockdep the page lock, anyway. Does it show up any of the > > buffered write deadlock possibilities? :) > > Not yet, it might just be that the concessions done to annotate this > type of lock were too severe. > > What I basically did was treat all the page locks as a single recursive > lock. Hmm, OK. There are a couple of page lock deadlocks there that wouldn't be picked up, but the page lock vs mmap_sem one probably should be. > > buffer lock is another notable bit-mutex that might be converted (I have > > the patch to do the similar nice !tas->trylock conversion for that too). > > I think it is used widely enough by tricky code that it would be useful > > to annotate as well. > > Not at all familiar with that lock, but yeah, we could have a look at > doing that too. Should be pretty well identical to the page lock. I'll cc you on the patch to do this equivalent API conversion, if you'd like. > > Unfortunately we can't convert bit_spinlock.h easily, I guess? > > Yeah, the space constraints make that rather hard. Each of these locks > needs some form of external meta-data. Yeah. > For the page lock I used one lock instance per file system type. Seems OK. - 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 05/12] mm: trylock_page
On Sunday 30 September 2007 01:01, Peter Zijlstra wrote: On Fri, 2007-09-28 at 13:11 +1000, Nick Piggin wrote: On Friday 28 September 2007 17:42, Peter Zijlstra wrote: Replace raw TestSetPageLocked() usage with trylock_page() I have such a thing queued too, for the lock bitops patches for when 2.6.24 opens, Andrew promises me :). I guess they should be identical, except I don't like doing trylock_page in place of SetPageLocked, for memory ordering performance and aesthetic reasons... I've got an init_page_locked (or set_page_locked... I can't remember, the patch is at home). Sure, that might work, or we could just make it so that add_to_*_cache is never passed an unlocked page. But sure... It does kind of make sense to have it there (because you want the page to be locked iff it gets inserted into pagecache). And wherever you lock the page, we'll still want an init_page_locked to be able to lock it while we are the only owner of it, for the same performance reason. Fine idea to lockdep the page lock, anyway. Does it show up any of the buffered write deadlock possibilities? :) Not yet, it might just be that the concessions done to annotate this type of lock were too severe. What I basically did was treat all the page locks as a single recursive lock. Hmm, OK. There are a couple of page lock deadlocks there that wouldn't be picked up, but the page lock vs mmap_sem one probably should be. buffer lock is another notable bit-mutex that might be converted (I have the patch to do the similar nice !tas-trylock conversion for that too). I think it is used widely enough by tricky code that it would be useful to annotate as well. Not at all familiar with that lock, but yeah, we could have a look at doing that too. Should be pretty well identical to the page lock. I'll cc you on the patch to do this equivalent API conversion, if you'd like. Unfortunately we can't convert bit_spinlock.h easily, I guess? Yeah, the space constraints make that rather hard. Each of these locks needs some form of external meta-data. Yeah. For the page lock I used one lock instance per file system type. Seems OK. - 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 05/12] mm: trylock_page
On Fri, 2007-09-28 at 13:11 +1000, Nick Piggin wrote: > On Friday 28 September 2007 17:42, Peter Zijlstra wrote: > > Replace raw TestSetPageLocked() usage with trylock_page() > > I have such a thing queued too, for the lock bitops patches for when 2.6.24 > opens, Andrew promises me :). > > I guess they should be identical, except I don't like doing trylock_page in > place of SetPageLocked, for memory ordering performance and aesthetic > reasons... I've got an init_page_locked (or set_page_locked... I can't > remember, the patch is at home). Sure, that might work, or we could just make it so that add_to_*_cache is never passed an unlocked page. But sure... > Fine idea to lockdep the page lock, anyway. Does it show up any of the > buffered write deadlock possibilities? :) Not yet, it might just be that the concessions done to annotate this type of lock were too severe. What I basically did was treat all the page locks as a single recursive lock. > buffer lock is another notable bit-mutex that might be converted (I have > the patch to do the similar nice !tas->trylock conversion for that too). I > think it is used widely enough by tricky code that it would be useful to > annotate as well. Not at all familiar with that lock, but yeah, we could have a look at doing that too. > Unfortunately we can't convert bit_spinlock.h easily, I guess? Yeah, the space constraints make that rather hard. Each of these locks needs some form of external meta-data. For the page lock I used one lock instance per file system type. - 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 05/12] mm: trylock_page
On Fri, 2007-09-28 at 13:11 +1000, Nick Piggin wrote: On Friday 28 September 2007 17:42, Peter Zijlstra wrote: Replace raw TestSetPageLocked() usage with trylock_page() I have such a thing queued too, for the lock bitops patches for when 2.6.24 opens, Andrew promises me :). I guess they should be identical, except I don't like doing trylock_page in place of SetPageLocked, for memory ordering performance and aesthetic reasons... I've got an init_page_locked (or set_page_locked... I can't remember, the patch is at home). Sure, that might work, or we could just make it so that add_to_*_cache is never passed an unlocked page. But sure... Fine idea to lockdep the page lock, anyway. Does it show up any of the buffered write deadlock possibilities? :) Not yet, it might just be that the concessions done to annotate this type of lock were too severe. What I basically did was treat all the page locks as a single recursive lock. buffer lock is another notable bit-mutex that might be converted (I have the patch to do the similar nice !tas-trylock conversion for that too). I think it is used widely enough by tricky code that it would be useful to annotate as well. Not at all familiar with that lock, but yeah, we could have a look at doing that too. Unfortunately we can't convert bit_spinlock.h easily, I guess? Yeah, the space constraints make that rather hard. Each of these locks needs some form of external meta-data. For the page lock I used one lock instance per file system type. - 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 05/12] mm: trylock_page
On Friday 28 September 2007 17:42, Peter Zijlstra wrote: > Replace raw TestSetPageLocked() usage with trylock_page() I have such a thing queued too, for the lock bitops patches for when 2.6.24 opens, Andrew promises me :). I guess they should be identical, except I don't like doing trylock_page in place of SetPageLocked, for memory ordering performance and aesthetic reasons... I've got an init_page_locked (or set_page_locked... I can't remember, the patch is at home). Fine idea to lockdep the page lock, anyway. Does it show up any of the buffered write deadlock possibilities? :) buffer lock is another notable bit-mutex that might be converted (I have the patch to do the similar nice !tas->trylock conversion for that too). I think it is used widely enough by tricky code that it would be useful to annotate as well. Unfortunately we can't convert bit_spinlock.h easily, I guess? - 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 05/12] mm: trylock_page
Replace raw TestSetPageLocked() usage with trylock_page()
Signed-off-by: Peter Zijlstra <[EMAIL PROTECTED]>
---
fs/afs/write.c |2 +-
fs/cifs/file.c |2 +-
fs/jbd/commit.c |2 +-
fs/jbd2/commit.c|2 +-
fs/splice.c |2 +-
fs/xfs/linux-2.6/xfs_aops.c |4 ++--
include/linux/pagemap.h |5 +
mm/filemap.c|4 ++--
mm/memory.c |2 +-
mm/migrate.c|4 ++--
mm/rmap.c |2 +-
mm/shmem.c |4 ++--
mm/swap.c |2 +-
mm/swap_state.c |2 +-
mm/swapfile.c |2 +-
mm/truncate.c |4 ++--
mm/vmscan.c |4 ++--
17 files changed, 27 insertions(+), 22 deletions(-)
Index: linux-2.6/fs/afs/write.c
===
--- linux-2.6.orig/fs/afs/write.c
+++ linux-2.6/fs/afs/write.c
@@ -404,7 +404,7 @@ static int afs_write_back_from_locked_pa
page = pages[loop];
if (page->index > wb->last)
break;
- if (TestSetPageLocked(page))
+ if (!trylock_page(page))
break;
if (!PageDirty(page) ||
page_private(page) != (unsigned long) wb) {
Index: linux-2.6/fs/cifs/file.c
===
--- linux-2.6.orig/fs/cifs/file.c
+++ linux-2.6/fs/cifs/file.c
@@ -1205,7 +1205,7 @@ retry:
if (first < 0)
lock_page(page);
- else if (TestSetPageLocked(page))
+ else if (!trylock_page(page))
break;
if (unlikely(page->mapping != mapping)) {
Index: linux-2.6/fs/jbd/commit.c
===
--- linux-2.6.orig/fs/jbd/commit.c
+++ linux-2.6/fs/jbd/commit.c
@@ -63,7 +63,7 @@ static void release_buffer_page(struct b
goto nope;
/* OK, it's a truncated page */
- if (TestSetPageLocked(page))
+ if (!trylock_page(page))
goto nope;
page_cache_get(page);
Index: linux-2.6/fs/jbd2/commit.c
===
--- linux-2.6.orig/fs/jbd2/commit.c
+++ linux-2.6/fs/jbd2/commit.c
@@ -63,7 +63,7 @@ static void release_buffer_page(struct b
goto nope;
/* OK, it's a truncated page */
- if (TestSetPageLocked(page))
+ if (!trylock_page(page))
goto nope;
page_cache_get(page);
Index: linux-2.6/fs/splice.c
===
--- linux-2.6.orig/fs/splice.c
+++ linux-2.6/fs/splice.c
@@ -364,7 +364,7 @@ __generic_file_splice_read(struct file *
* for an in-flight io page
*/
if (flags & SPLICE_F_NONBLOCK) {
- if (TestSetPageLocked(page))
+ if (!trylock_page(page))
break;
} else
lock_page(page);
Index: linux-2.6/fs/xfs/linux-2.6/xfs_aops.c
===
--- linux-2.6.orig/fs/xfs/linux-2.6/xfs_aops.c
+++ linux-2.6/fs/xfs/linux-2.6/xfs_aops.c
@@ -665,7 +665,7 @@ xfs_probe_cluster(
} else
pg_offset = PAGE_CACHE_SIZE;
- if (page->index == tindex && !TestSetPageLocked(page)) {
+ if (page->index == tindex && trylock_page(page)) {
pg_len = xfs_probe_page(page, pg_offset,
mapped);
unlock_page(page);
}
@@ -749,7 +749,7 @@ xfs_convert_page(
if (page->index != tindex)
goto fail;
- if (TestSetPageLocked(page))
+ if (!trylock_page(page))
goto fail;
if (PageWriteback(page))
goto fail_unlock_page;
Index: linux-2.6/include/linux/pagemap.h
===
--- linux-2.6.orig/include/linux/pagemap.h
+++ linux-2.6/include/linux/pagemap.h
@@ -167,6 +167,11 @@ static inline void lock_page(struct page
__lock_page(page);
}
+static inline int trylock_page(struct page *page)
+{
+ return !TestSetPageLocked(page);
+}
+
/*
* lock_page_nosync should only be used if we can't pin the page's inode.
* Doesn't play quite so well with block device plugging.
Index: linux-2.6/mm/filemap.c
===
--- linux-2.6.orig/mm/filemap.c
+++
[PATCH 05/12] mm: trylock_page
Replace raw TestSetPageLocked() usage with trylock_page()
Signed-off-by: Peter Zijlstra [EMAIL PROTECTED]
---
fs/afs/write.c |2 +-
fs/cifs/file.c |2 +-
fs/jbd/commit.c |2 +-
fs/jbd2/commit.c|2 +-
fs/splice.c |2 +-
fs/xfs/linux-2.6/xfs_aops.c |4 ++--
include/linux/pagemap.h |5 +
mm/filemap.c|4 ++--
mm/memory.c |2 +-
mm/migrate.c|4 ++--
mm/rmap.c |2 +-
mm/shmem.c |4 ++--
mm/swap.c |2 +-
mm/swap_state.c |2 +-
mm/swapfile.c |2 +-
mm/truncate.c |4 ++--
mm/vmscan.c |4 ++--
17 files changed, 27 insertions(+), 22 deletions(-)
Index: linux-2.6/fs/afs/write.c
===
--- linux-2.6.orig/fs/afs/write.c
+++ linux-2.6/fs/afs/write.c
@@ -404,7 +404,7 @@ static int afs_write_back_from_locked_pa
page = pages[loop];
if (page-index wb-last)
break;
- if (TestSetPageLocked(page))
+ if (!trylock_page(page))
break;
if (!PageDirty(page) ||
page_private(page) != (unsigned long) wb) {
Index: linux-2.6/fs/cifs/file.c
===
--- linux-2.6.orig/fs/cifs/file.c
+++ linux-2.6/fs/cifs/file.c
@@ -1205,7 +1205,7 @@ retry:
if (first 0)
lock_page(page);
- else if (TestSetPageLocked(page))
+ else if (!trylock_page(page))
break;
if (unlikely(page-mapping != mapping)) {
Index: linux-2.6/fs/jbd/commit.c
===
--- linux-2.6.orig/fs/jbd/commit.c
+++ linux-2.6/fs/jbd/commit.c
@@ -63,7 +63,7 @@ static void release_buffer_page(struct b
goto nope;
/* OK, it's a truncated page */
- if (TestSetPageLocked(page))
+ if (!trylock_page(page))
goto nope;
page_cache_get(page);
Index: linux-2.6/fs/jbd2/commit.c
===
--- linux-2.6.orig/fs/jbd2/commit.c
+++ linux-2.6/fs/jbd2/commit.c
@@ -63,7 +63,7 @@ static void release_buffer_page(struct b
goto nope;
/* OK, it's a truncated page */
- if (TestSetPageLocked(page))
+ if (!trylock_page(page))
goto nope;
page_cache_get(page);
Index: linux-2.6/fs/splice.c
===
--- linux-2.6.orig/fs/splice.c
+++ linux-2.6/fs/splice.c
@@ -364,7 +364,7 @@ __generic_file_splice_read(struct file *
* for an in-flight io page
*/
if (flags SPLICE_F_NONBLOCK) {
- if (TestSetPageLocked(page))
+ if (!trylock_page(page))
break;
} else
lock_page(page);
Index: linux-2.6/fs/xfs/linux-2.6/xfs_aops.c
===
--- linux-2.6.orig/fs/xfs/linux-2.6/xfs_aops.c
+++ linux-2.6/fs/xfs/linux-2.6/xfs_aops.c
@@ -665,7 +665,7 @@ xfs_probe_cluster(
} else
pg_offset = PAGE_CACHE_SIZE;
- if (page-index == tindex !TestSetPageLocked(page)) {
+ if (page-index == tindex trylock_page(page)) {
pg_len = xfs_probe_page(page, pg_offset,
mapped);
unlock_page(page);
}
@@ -749,7 +749,7 @@ xfs_convert_page(
if (page-index != tindex)
goto fail;
- if (TestSetPageLocked(page))
+ if (!trylock_page(page))
goto fail;
if (PageWriteback(page))
goto fail_unlock_page;
Index: linux-2.6/include/linux/pagemap.h
===
--- linux-2.6.orig/include/linux/pagemap.h
+++ linux-2.6/include/linux/pagemap.h
@@ -167,6 +167,11 @@ static inline void lock_page(struct page
__lock_page(page);
}
+static inline int trylock_page(struct page *page)
+{
+ return !TestSetPageLocked(page);
+}
+
/*
* lock_page_nosync should only be used if we can't pin the page's inode.
* Doesn't play quite so well with block device plugging.
Index: linux-2.6/mm/filemap.c
===
--- linux-2.6.orig/mm/filemap.c
+++
Re: [PATCH 05/12] mm: trylock_page
On Friday 28 September 2007 17:42, Peter Zijlstra wrote: Replace raw TestSetPageLocked() usage with trylock_page() I have such a thing queued too, for the lock bitops patches for when 2.6.24 opens, Andrew promises me :). I guess they should be identical, except I don't like doing trylock_page in place of SetPageLocked, for memory ordering performance and aesthetic reasons... I've got an init_page_locked (or set_page_locked... I can't remember, the patch is at home). Fine idea to lockdep the page lock, anyway. Does it show up any of the buffered write deadlock possibilities? :) buffer lock is another notable bit-mutex that might be converted (I have the patch to do the similar nice !tas-trylock conversion for that too). I think it is used widely enough by tricky code that it would be useful to annotate as well. Unfortunately we can't convert bit_spinlock.h easily, I guess? - 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/
