* [PATCH 0/2] z3fold fixes
@ 2016-11-26 19:15 Vitaly Wool
2016-11-26 19:17 ` [PATCH 1/2] z3fold: fix header size related issues Vitaly Wool
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Vitaly Wool @ 2016-11-26 19:15 UTC (permalink / raw)
To: Linux-MM, linux-kernel
Cc: Dan Streetman, Andrew Morton, Arnd Bergmann, Dan Carpenter
Here come 2 patches with z3fold fixes for chunks counting and locking. As commit 50a50d2 ("z3fold: don't fail kernel build is z3fold_header is too big") was NAK'ed [1], I would suggest that we removed that one and the next z3fold commit cc1e9c8 ("z3fold: discourage use of pages that weren't compacted") and applied the coming 2 instead.
Signed-off-by: Vitaly Wool <vitalywool@gmail.com>
[1] https://lkml.org/lkml/2016/11/25/595
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/2] z3fold: fix header size related issues 2016-11-26 19:15 [PATCH 0/2] z3fold fixes Vitaly Wool @ 2016-11-26 19:17 ` Vitaly Wool 2016-11-26 19:21 ` [PATCH 2/2] z3fold: fix locking issues Vitaly Wool 2016-11-29 22:33 ` [PATCH 0/2] z3fold fixes Dan Streetman 2 siblings, 0 replies; 9+ messages in thread From: Vitaly Wool @ 2016-11-26 19:17 UTC (permalink / raw) To: Linux-MM, linux-kernel Cc: Dan Streetman, Andrew Morton, Arnd Bergmann, Dan Carpenter Currently the whole kernel build will be stopped if the size of struct z3fold_header is greater than the size of one chunk, which is 64 bytes by default. This patch instead defines the offset for z3fold objects as the size of the z3fold header in chunks. Fixed also are the calculation of num_free_chunks() and the address to move the middle chunk to in case of in-page compaction in z3fold_compact_page(). This patch is to be applied on top of commit 570931c ("z3fold: use per-page spinlock"), substituting commit 50a50d2 ("z3fold: don't fail kernel build is z3fold_header is too big"). Signed-off-by: Vitaly Wool <vitalywool@gmail.com> --- mm/z3fold.c | 161 ++++++++++++++++++++++++++++++++---------------------------- 1 file changed, 87 insertions(+), 74 deletions(-) diff --git a/mm/z3fold.c b/mm/z3fold.c index 7ad70fa..efbcfcc 100644 --- a/mm/z3fold.c +++ b/mm/z3fold.c @@ -34,29 +34,60 @@ /***************** * Structures *****************/ +struct z3fold_pool; +struct z3fold_ops { + int (*evict)(struct z3fold_pool *pool, unsigned long handle); +}; + +enum buddy { + HEADLESS = 0, + FIRST, + MIDDLE, + LAST, + BUDDIES_MAX +}; + +/* + * struct z3fold_header - z3fold page metadata occupying the first chunk of each + * z3fold page, except for HEADLESS pages + * @buddy: links the z3fold page into the relevant list in the pool + * @page_lock: per-page lock + * @first_chunks: the size of the first buddy in chunks, 0 if free + * @middle_chunks: the size of the middle buddy in chunks, 0 if free + * @last_chunks: the size of the last buddy in chunks, 0 if free + * @first_num: the starting number (for the first handle) + */ +struct z3fold_header { + struct list_head buddy; + raw_spinlock_t page_lock; + unsigned short first_chunks; + unsigned short middle_chunks; + unsigned short last_chunks; + unsigned short start_middle; + unsigned short first_num:2; +}; + /* * NCHUNKS_ORDER determines the internal allocation granularity, effectively * adjusting internal fragmentation. It also determines the number of * freelists maintained in each pool. NCHUNKS_ORDER of 6 means that the - * allocation granularity will be in chunks of size PAGE_SIZE/64. As one chunk - * in allocated page is occupied by z3fold header, NCHUNKS will be calculated - * to 63 which shows the max number of free chunks in z3fold page, also there - * will be 63 freelists per pool. + * allocation granularity will be in chunks of size PAGE_SIZE/64. Some chunks + * in the beginning of an allocated page are occupied by z3fold header, so + * NCHUNKS will be calculated to 63 (or 62 in case CONFIG_DEBUG_SPINLOCK=y), + * which shows the max number of free chunks in z3fold page, also there will + * be 63, or 62, respectively, freelists per pool. */ #define NCHUNKS_ORDER 6 #define CHUNK_SHIFT (PAGE_SHIFT - NCHUNKS_ORDER) #define CHUNK_SIZE (1 << CHUNK_SHIFT) -#define ZHDR_SIZE_ALIGNED CHUNK_SIZE +#define ZHDR_SIZE_ALIGNED round_up(sizeof(struct z3fold_header), CHUNK_SIZE) +#define ZHDR_CHUNKS (ZHDR_SIZE_ALIGNED >> CHUNK_SHIFT) +#define TOTAL_CHUNKS (PAGE_SIZE >> CHUNK_SHIFT) #define NCHUNKS ((PAGE_SIZE - ZHDR_SIZE_ALIGNED) >> CHUNK_SHIFT) #define BUDDY_MASK (0x3) -struct z3fold_pool; -struct z3fold_ops { - int (*evict)(struct z3fold_pool *pool, unsigned long handle); -}; - /** * struct z3fold_pool - stores metadata for each z3fold pool * @lock: protects all pool fields and first|last_chunk fields of any @@ -86,33 +117,6 @@ struct z3fold_pool { const struct zpool_ops *zpool_ops; }; -enum buddy { - HEADLESS = 0, - FIRST, - MIDDLE, - LAST, - BUDDIES_MAX -}; - -/* - * struct z3fold_header - z3fold page metadata occupying the first chunk of each - * z3fold page, except for HEADLESS pages - * @buddy: links the z3fold page into the relevant list in the pool - * @page_lock: per-page lock - * @first_chunks: the size of the first buddy in chunks, 0 if free - * @middle_chunks: the size of the middle buddy in chunks, 0 if free - * @last_chunks: the size of the last buddy in chunks, 0 if free - * @first_num: the starting number (for the first handle) - */ -struct z3fold_header { - struct list_head buddy; - raw_spinlock_t page_lock; - unsigned short first_chunks; - unsigned short middle_chunks; - unsigned short last_chunks; - unsigned short start_middle; - unsigned short first_num:2; -}; /* * Internal z3fold page flags @@ -123,6 +127,7 @@ enum z3fold_page_flags { MIDDLE_CHUNK_MAPPED, }; + /***************** * Helpers *****************/ @@ -220,9 +225,10 @@ static int num_free_chunks(struct z3fold_header *zhdr) */ if (zhdr->middle_chunks != 0) { int nfree_before = zhdr->first_chunks ? - 0 : zhdr->start_middle - 1; + 0 : zhdr->start_middle - ZHDR_CHUNKS; int nfree_after = zhdr->last_chunks ? - 0 : NCHUNKS - zhdr->start_middle - zhdr->middle_chunks; + 0 : TOTAL_CHUNKS - + (zhdr->start_middle + zhdr->middle_chunks); nfree = max(nfree_before, nfree_after); } else nfree = NCHUNKS - zhdr->first_chunks - zhdr->last_chunks; @@ -287,40 +293,47 @@ static int z3fold_compact_page(struct z3fold_header *zhdr) int ret = 0; if (test_bit(MIDDLE_CHUNK_MAPPED, &page->private)) + goto out; /* can't move middle chunk, it's used */ + + if (zhdr->middle_chunks == 0) + goto out; /* nothing to compact */ + + if (zhdr->first_chunks == 0 && zhdr->last_chunks == 0) { + /* move to the beginning */ + mchunk_memmove(zhdr, ZHDR_CHUNKS); + zhdr->first_chunks = zhdr->middle_chunks; + zhdr->middle_chunks = 0; + zhdr->start_middle = 0; + zhdr->first_num++; + ret = 1; goto out; + } - if (zhdr->middle_chunks != 0) { - if (zhdr->first_chunks == 0 && zhdr->last_chunks == 0) { - mchunk_memmove(zhdr, 1); /* move to the beginning */ - zhdr->first_chunks = zhdr->middle_chunks; - zhdr->middle_chunks = 0; - zhdr->start_middle = 0; - zhdr->first_num++; - ret = 1; - goto out; - } - - /* - * moving data is expensive, so let's only do that if - * there's substantial gain (at least BIG_CHUNK_GAP chunks) - */ - if (zhdr->first_chunks != 0 && zhdr->last_chunks == 0 && - zhdr->start_middle > zhdr->first_chunks + BIG_CHUNK_GAP) { - mchunk_memmove(zhdr, zhdr->first_chunks + 1); - zhdr->start_middle = zhdr->first_chunks + 1; - ret = 1; - goto out; - } - if (zhdr->last_chunks != 0 && zhdr->first_chunks == 0 && - zhdr->middle_chunks + zhdr->last_chunks <= - NCHUNKS - zhdr->start_middle - BIG_CHUNK_GAP) { - unsigned short new_start = NCHUNKS - zhdr->last_chunks - - zhdr->middle_chunks; - mchunk_memmove(zhdr, new_start); - zhdr->start_middle = new_start; - ret = 1; - goto out; - } + /* + * moving data is expensive, so let's only do that if + * there's substantial gain (at least BIG_CHUNK_GAP chunks) + */ + if (zhdr->first_chunks != 0 && zhdr->last_chunks == 0 && + zhdr->start_middle - (zhdr->first_chunks + ZHDR_CHUNKS) >= + BIG_CHUNK_GAP) { + /* new_start: right after 1st chunk */ + unsigned short new_start = zhdr->first_chunks + ZHDR_CHUNKS; + mchunk_memmove(zhdr, new_start); + zhdr->start_middle = new_start; + ret = 1; + goto out; + } + if (zhdr->last_chunks != 0 && zhdr->first_chunks == 0 && + TOTAL_CHUNKS - + (zhdr->last_chunks + zhdr->start_middle + zhdr->middle_chunks) >= + BIG_CHUNK_GAP) { + /* new_start: right before last chunk */ + unsigned short new_start = TOTAL_CHUNKS - + (zhdr->last_chunks + zhdr->middle_chunks); + mchunk_memmove(zhdr, new_start); + zhdr->start_middle = new_start; + ret = 1; + goto out; } out: return ret; @@ -425,7 +438,7 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp, zhdr->last_chunks = chunks; else { zhdr->middle_chunks = chunks; - zhdr->start_middle = zhdr->first_chunks + 1; + zhdr->start_middle = zhdr->first_chunks + ZHDR_CHUNKS; } spin_lock(&pool->lock); @@ -870,8 +883,8 @@ MODULE_ALIAS("zpool-z3fold"); static int __init init_z3fold(void) { - /* Make sure the z3fold header will fit in one chunk */ - BUILD_BUG_ON(sizeof(struct z3fold_header) > ZHDR_SIZE_ALIGNED); + /* Make sure the z3fold header is not larger than the page size */ + BUILD_BUG_ON(ZHDR_SIZE_ALIGNED > PAGE_SIZE); zpool_register_driver(&z3fold_zpool_driver); return 0; -- 2.4.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] z3fold: fix locking issues 2016-11-26 19:15 [PATCH 0/2] z3fold fixes Vitaly Wool 2016-11-26 19:17 ` [PATCH 1/2] z3fold: fix header size related issues Vitaly Wool @ 2016-11-26 19:21 ` Vitaly Wool 2016-11-29 22:33 ` [PATCH 0/2] z3fold fixes Dan Streetman 2 siblings, 0 replies; 9+ messages in thread From: Vitaly Wool @ 2016-11-26 19:21 UTC (permalink / raw) To: Linux-MM, linux-kernel Cc: Dan Streetman, Andrew Morton, Arnd Bergmann, Dan Carpenter Commit 570931c ("z3fold: use per-page spinlock") introduced locking issues in reclaim function reported in [1] and [2]. This patch addresses these issues, also fixing the check for empty lru list (it was only checked once, while it should be checked every time we want to get the last lru entry). [1] https://lkml.org/lkml/2016/11/25/628 [2] http://www.spinics.net/lists/linux-mm/msg117227.html Signed-off-by: Vitaly Wool <vitalywool@gmail.com> --- mm/z3fold.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/mm/z3fold.c b/mm/z3fold.c index efbcfcc..729a2da 100644 --- a/mm/z3fold.c +++ b/mm/z3fold.c @@ -607,12 +607,15 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int retries) unsigned long first_handle = 0, middle_handle = 0, last_handle = 0; spin_lock(&pool->lock); - if (!pool->ops || !pool->ops->evict || list_empty(&pool->lru) || - retries == 0) { + if (!pool->ops || !pool->ops->evict || retries == 0) { spin_unlock(&pool->lock); return -EINVAL; } for (i = 0; i < retries; i++) { + if (list_empty(&pool->lru)) { + spin_unlock(&pool->lock); + return -EINVAL; + } page = list_last_entry(&pool->lru, struct page, lru); list_del(&page->lru); @@ -671,8 +674,7 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int retries) * All buddies are now free, free the z3fold page and * return success. */ - clear_bit(PAGE_HEADLESS, &page->private); - if (!test_bit(PAGE_HEADLESS, &page->private)) + if (!test_and_clear_bit(PAGE_HEADLESS, &page->private)) z3fold_page_unlock(zhdr); free_z3fold_page(zhdr); atomic64_dec(&pool->pages_nr); @@ -684,6 +686,7 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int retries) /* Full, add to buddied list */ spin_lock(&pool->lock); list_add(&zhdr->buddy, &pool->buddied); + spin_unlock(&pool->lock); } else { z3fold_compact_page(zhdr); /* add to unbuddied list */ @@ -691,15 +694,18 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int retries) freechunks = num_free_chunks(zhdr); list_add(&zhdr->buddy, &pool->unbuddied[freechunks]); + spin_unlock(&pool->lock); } } + if (!test_bit(PAGE_HEADLESS, &page->private)) + z3fold_page_unlock(zhdr); + + spin_lock(&pool->lock); /* add to beginning of LRU */ list_add(&page->lru, &pool->lru); } spin_unlock(&pool->lock); - if (!test_bit(PAGE_HEADLESS, &page->private)) - z3fold_page_unlock(zhdr); return -EAGAIN; } -- 2.4.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] z3fold fixes 2016-11-26 19:15 [PATCH 0/2] z3fold fixes Vitaly Wool 2016-11-26 19:17 ` [PATCH 1/2] z3fold: fix header size related issues Vitaly Wool 2016-11-26 19:21 ` [PATCH 2/2] z3fold: fix locking issues Vitaly Wool @ 2016-11-29 22:33 ` Dan Streetman 2016-11-29 22:39 ` Andrew Morton 2 siblings, 1 reply; 9+ messages in thread From: Dan Streetman @ 2016-11-29 22:33 UTC (permalink / raw) To: Vitaly Wool Cc: Linux-MM, linux-kernel, Andrew Morton, Arnd Bergmann, Dan Carpenter On Sat, Nov 26, 2016 at 2:15 PM, Vitaly Wool <vitalywool@gmail.com> wrote: > Here come 2 patches with z3fold fixes for chunks counting and locking. As commit 50a50d2 ("z3fold: don't fail kernel build is z3fold_header is too big") was NAK'ed [1], I would suggest that we removed that one and the next z3fold commit cc1e9c8 ("z3fold: discourage use of pages that weren't compacted") and applied the coming 2 instead. Instead of adding these onto all the previous ones, could you redo the entire z3fold series? I think it'll be simpler to review the series all at once and that would remove some of the stuff from previous patches that shouldn't be there. If that's ok with Andrew, of course, but I don't think any of the z3fold patches have been pushed to Linus yet. > > Signed-off-by: Vitaly Wool <vitalywool@gmail.com> > > [1] https://lkml.org/lkml/2016/11/25/595 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] z3fold fixes 2016-11-29 22:33 ` [PATCH 0/2] z3fold fixes Dan Streetman @ 2016-11-29 22:39 ` Andrew Morton 2016-12-18 8:15 ` Vitaly Wool 0 siblings, 1 reply; 9+ messages in thread From: Andrew Morton @ 2016-11-29 22:39 UTC (permalink / raw) To: Dan Streetman Cc: Vitaly Wool, Linux-MM, linux-kernel, Arnd Bergmann, Dan Carpenter On Tue, 29 Nov 2016 17:33:19 -0500 Dan Streetman <ddstreet@ieee.org> wrote: > On Sat, Nov 26, 2016 at 2:15 PM, Vitaly Wool <vitalywool@gmail.com> wrote: > > Here come 2 patches with z3fold fixes for chunks counting and locking. As commit 50a50d2 ("z3fold: don't fail kernel build is z3fold_header is too big") was NAK'ed [1], I would suggest that we removed that one and the next z3fold commit cc1e9c8 ("z3fold: discourage use of pages that weren't compacted") and applied the coming 2 instead. > > Instead of adding these onto all the previous ones, could you redo the > entire z3fold series? I think it'll be simpler to review the series > all at once and that would remove some of the stuff from previous > patches that shouldn't be there. > > If that's ok with Andrew, of course, but I don't think any of the > z3fold patches have been pushed to Linus yet. Sounds good to me. I had a few surprise rejects when merging these two, which indicates that things might be out of sync. I presently have: z3fold-limit-first_num-to-the-actual-range-of-possible-buddy-indexes.patch z3fold-make-pages_nr-atomic.patch z3fold-extend-compaction-function.patch z3fold-use-per-page-spinlock.patch z3fold-discourage-use-of-pages-that-werent-compacted.patch z3fold-fix-header-size-related-issues.patch z3fold-fix-locking-issues.patch ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] z3fold fixes 2016-11-29 22:39 ` Andrew Morton @ 2016-12-18 8:15 ` Vitaly Wool 2016-12-22 21:55 ` Dan Streetman 0 siblings, 1 reply; 9+ messages in thread From: Vitaly Wool @ 2016-12-18 8:15 UTC (permalink / raw) To: Andrew Morton Cc: Dan Streetman, Linux-MM, linux-kernel, Arnd Bergmann, Dan Carpenter On Tue, Nov 29, 2016 at 11:39 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Tue, 29 Nov 2016 17:33:19 -0500 Dan Streetman <ddstreet@ieee.org> wrote: > >> On Sat, Nov 26, 2016 at 2:15 PM, Vitaly Wool <vitalywool@gmail.com> wrote: >> > Here come 2 patches with z3fold fixes for chunks counting and locking. As commit 50a50d2 ("z3fold: don't fail kernel build is z3fold_header is too big") was NAK'ed [1], I would suggest that we removed that one and the next z3fold commit cc1e9c8 ("z3fold: discourage use of pages that weren't compacted") and applied the coming 2 instead. >> >> Instead of adding these onto all the previous ones, could you redo the >> entire z3fold series? I think it'll be simpler to review the series >> all at once and that would remove some of the stuff from previous >> patches that shouldn't be there. >> >> If that's ok with Andrew, of course, but I don't think any of the >> z3fold patches have been pushed to Linus yet. > > Sounds good to me. I had a few surprise rejects when merging these > two, which indicates that things might be out of sync. > > I presently have: > > z3fold-limit-first_num-to-the-actual-range-of-possible-buddy-indexes.patch > z3fold-make-pages_nr-atomic.patch > z3fold-extend-compaction-function.patch > z3fold-use-per-page-spinlock.patch > z3fold-discourage-use-of-pages-that-werent-compacted.patch > z3fold-fix-header-size-related-issues.patch > z3fold-fix-locking-issues.patch My initial suggestion was to have it the following way: z3fold-limit-first_num-to-the-actual-range-of-possible-buddy-indexes.patch z3fold-make-pages_nr-atomic.patch z3fold-extend-compaction-function.patch z3fold-use-per-page-spinlock.patch z3fold-fix-header-size-related-issues.patch z3fold-fix-locking-issues.patch I would prefer to keep the fix-XXX patches separate since e. g. z3fold-fix-header-size-related-issues.patch concerns also the problems that have been in the code for a while now. I am ok with folding these into the relevant main patches but once again, given that some fixes are related to the code that is already merged, I don't see why it would be better. ~vitaly ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] z3fold fixes 2016-12-18 8:15 ` Vitaly Wool @ 2016-12-22 21:55 ` Dan Streetman 2016-12-22 23:04 ` Vitaly Wool 0 siblings, 1 reply; 9+ messages in thread From: Dan Streetman @ 2016-12-22 21:55 UTC (permalink / raw) To: Vitaly Wool Cc: Andrew Morton, Linux-MM, linux-kernel, Arnd Bergmann, Dan Carpenter On Sun, Dec 18, 2016 at 3:15 AM, Vitaly Wool <vitalywool@gmail.com> wrote: > On Tue, Nov 29, 2016 at 11:39 PM, Andrew Morton > <akpm@linux-foundation.org> wrote: >> On Tue, 29 Nov 2016 17:33:19 -0500 Dan Streetman <ddstreet@ieee.org> wrote: >> >>> On Sat, Nov 26, 2016 at 2:15 PM, Vitaly Wool <vitalywool@gmail.com> wrote: >>> > Here come 2 patches with z3fold fixes for chunks counting and locking. As commit 50a50d2 ("z3fold: don't fail kernel build is z3fold_header is too big") was NAK'ed [1], I would suggest that we removed that one and the next z3fold commit cc1e9c8 ("z3fold: discourage use of pages that weren't compacted") and applied the coming 2 instead. >>> >>> Instead of adding these onto all the previous ones, could you redo the >>> entire z3fold series? I think it'll be simpler to review the series >>> all at once and that would remove some of the stuff from previous >>> patches that shouldn't be there. >>> >>> If that's ok with Andrew, of course, but I don't think any of the >>> z3fold patches have been pushed to Linus yet. >> >> Sounds good to me. I had a few surprise rejects when merging these >> two, which indicates that things might be out of sync. >> >> I presently have: >> >> z3fold-limit-first_num-to-the-actual-range-of-possible-buddy-indexes.patch >> z3fold-make-pages_nr-atomic.patch >> z3fold-extend-compaction-function.patch >> z3fold-use-per-page-spinlock.patch >> z3fold-discourage-use-of-pages-that-werent-compacted.patch >> z3fold-fix-header-size-related-issues.patch >> z3fold-fix-locking-issues.patch > > My initial suggestion was to have it the following way: > z3fold-limit-first_num-to-the-actual-range-of-possible-buddy-indexes.patch this is a good one, acked by both of us; it should stay and go upstream to Linus > z3fold-make-pages_nr-atomic.patch the change itself looks ok and I acked it, but as Andrew commented the log says nothing about why it's being changed; the atomic function is slower so the log should explain why it's being changed; anyone reviewing the log history won't know why you made the change, and the change all by itself is a step backwards in performance. > z3fold-extend-compaction-function.patch this explictly has a bug in it that's fixed in one of the later patches; instead, this should be fixed up and resent. > z3fold-use-per-page-spinlock.patch i should have explicitly nak'ed this, as not only did it add a bug (fixed by the the other 'fix-' patch below) but its design should be replaced by kref counting, which your latest patch is working towards... > z3fold-fix-header-size-related-issues.patch > z3fold-fix-locking-issues.patch and these fix the known problems in the previous patches. > > I would prefer to keep the fix-XXX patches separate since e. g. > z3fold-fix-header-size-related-issues.patch concerns also the problems > that have been in the code for a while now. I am ok with folding these > into the relevant main patches but once again, given that some fixes > are related to the code that is already merged, I don't see why it > would be better. none of those patches are "merged", the last z3fold patch in Linus' tree is 43afc194 from June. Just because they're in Andrew's mmotm queue (and/or linux-next) doesn't mean they are going to be merged...(correct me please if I'm wrong there Andrew) So as you can see by my patch-by-patch breakdown, almost all of them need changes based on feedback from various people. And they are all related - your goal is to improve z3fold performance, right? IMHO they should be sent as a single patch series with that goal in the cover letter, including specific details and numbers about how the series does improve performance. > > ~vitaly > > -- > 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 hrefmailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] z3fold fixes 2016-12-22 21:55 ` Dan Streetman @ 2016-12-22 23:04 ` Vitaly Wool 2016-12-23 13:09 ` Dan Streetman 0 siblings, 1 reply; 9+ messages in thread From: Vitaly Wool @ 2016-12-22 23:04 UTC (permalink / raw) To: Dan Streetman Cc: Andrew Morton, Linux-MM, linux-kernel, Arnd Bergmann, Dan Carpenter On Thu, Dec 22, 2016 at 10:55 PM, Dan Streetman <ddstreet@ieee.org> wrote: > On Sun, Dec 18, 2016 at 3:15 AM, Vitaly Wool <vitalywool@gmail.com> wrote: >> On Tue, Nov 29, 2016 at 11:39 PM, Andrew Morton >> <akpm@linux-foundation.org> wrote: >>> On Tue, 29 Nov 2016 17:33:19 -0500 Dan Streetman <ddstreet@ieee.org> wrote: >>> >>>> On Sat, Nov 26, 2016 at 2:15 PM, Vitaly Wool <vitalywool@gmail.com> wrote: >>>> > Here come 2 patches with z3fold fixes for chunks counting and locking. As commit 50a50d2 ("z3fold: don't fail kernel build is z3fold_header is too big") was NAK'ed [1], I would suggest that we removed that one and the next z3fold commit cc1e9c8 ("z3fold: discourage use of pages that weren't compacted") and applied the coming 2 instead. >>>> >>>> Instead of adding these onto all the previous ones, could you redo the >>>> entire z3fold series? I think it'll be simpler to review the series >>>> all at once and that would remove some of the stuff from previous >>>> patches that shouldn't be there. >>>> >>>> If that's ok with Andrew, of course, but I don't think any of the >>>> z3fold patches have been pushed to Linus yet. >>> >>> Sounds good to me. I had a few surprise rejects when merging these >>> two, which indicates that things might be out of sync. >>> >>> I presently have: >>> >>> z3fold-limit-first_num-to-the-actual-range-of-possible-buddy-indexes.patch >>> z3fold-make-pages_nr-atomic.patch >>> z3fold-extend-compaction-function.patch >>> z3fold-use-per-page-spinlock.patch >>> z3fold-discourage-use-of-pages-that-werent-compacted.patch >>> z3fold-fix-header-size-related-issues.patch >>> z3fold-fix-locking-issues.patch >> >> My initial suggestion was to have it the following way: >> z3fold-limit-first_num-to-the-actual-range-of-possible-buddy-indexes.patch > > this is a good one, acked by both of us; it should stay and go upstream to Linus > >> z3fold-make-pages_nr-atomic.patch > > the change itself looks ok and I acked it, but as Andrew commented the > log says nothing about why it's being changed; the atomic function is > slower so the log should explain why it's being changed; anyone > reviewing the log history won't know why you made the change, and the > change all by itself is a step backwards in performance. > >> z3fold-extend-compaction-function.patch > > this explictly has a bug in it that's fixed in one of the later > patches; instead, this should be fixed up and resent. > >> z3fold-use-per-page-spinlock.patch > > i should have explicitly nak'ed this, as not only did it add a bug > (fixed by the the other 'fix-' patch below) but its design should be > replaced by kref counting, which your latest patch is working > towards... > >> z3fold-fix-header-size-related-issues.patch >> z3fold-fix-locking-issues.patch > > and these fix the known problems in the previous patches. > >> >> I would prefer to keep the fix-XXX patches separate since e. g. >> z3fold-fix-header-size-related-issues.patch concerns also the problems >> that have been in the code for a while now. I am ok with folding these >> into the relevant main patches but once again, given that some fixes >> are related to the code that is already merged, I don't see why it >> would be better. > > none of those patches are "merged", the last z3fold patch in Linus' > tree is 43afc194 from June. Just because they're in Andrew's mmotm > queue (and/or linux-next) doesn't mean they are going to be > merged...(correct me please if I'm wrong there Andrew) that I do understand, however, z3fold-fix-header-size-related-issues.patch fixes the off-by-one issue present in the code that is in Linus's tree too. > So as you can see by my patch-by-patch breakdown, almost all of them > need changes based on feedback from various people. And they are all > related - your goal is to improve z3fold performance, right? IMHO > they should be sent as a single patch series with that goal in the > cover letter, including specific details and numbers about how the > series does improve performance. but that is a good idea anyway, the only thing i\m not sure about is whether it makes sense to fold z3fold-fix-header-size-related-issues.patch into another or not. ~vitaly ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] z3fold fixes 2016-12-22 23:04 ` Vitaly Wool @ 2016-12-23 13:09 ` Dan Streetman 0 siblings, 0 replies; 9+ messages in thread From: Dan Streetman @ 2016-12-23 13:09 UTC (permalink / raw) To: Vitaly Wool Cc: Andrew Morton, Linux-MM, linux-kernel, Arnd Bergmann, Dan Carpenter On Thu, Dec 22, 2016 at 6:04 PM, Vitaly Wool <vitalywool@gmail.com> wrote: > On Thu, Dec 22, 2016 at 10:55 PM, Dan Streetman <ddstreet@ieee.org> wrote: >> On Sun, Dec 18, 2016 at 3:15 AM, Vitaly Wool <vitalywool@gmail.com> wrote: >>> On Tue, Nov 29, 2016 at 11:39 PM, Andrew Morton >>> <akpm@linux-foundation.org> wrote: >>>> On Tue, 29 Nov 2016 17:33:19 -0500 Dan Streetman <ddstreet@ieee.org> wrote: >>>> >>>>> On Sat, Nov 26, 2016 at 2:15 PM, Vitaly Wool <vitalywool@gmail.com> wrote: >>>>> > Here come 2 patches with z3fold fixes for chunks counting and locking. As commit 50a50d2 ("z3fold: don't fail kernel build is z3fold_header is too big") was NAK'ed [1], I would suggest that we removed that one and the next z3fold commit cc1e9c8 ("z3fold: discourage use of pages that weren't compacted") and applied the coming 2 instead. >>>>> >>>>> Instead of adding these onto all the previous ones, could you redo the >>>>> entire z3fold series? I think it'll be simpler to review the series >>>>> all at once and that would remove some of the stuff from previous >>>>> patches that shouldn't be there. >>>>> >>>>> If that's ok with Andrew, of course, but I don't think any of the >>>>> z3fold patches have been pushed to Linus yet. >>>> >>>> Sounds good to me. I had a few surprise rejects when merging these >>>> two, which indicates that things might be out of sync. >>>> >>>> I presently have: >>>> >>>> z3fold-limit-first_num-to-the-actual-range-of-possible-buddy-indexes.patch >>>> z3fold-make-pages_nr-atomic.patch >>>> z3fold-extend-compaction-function.patch >>>> z3fold-use-per-page-spinlock.patch >>>> z3fold-discourage-use-of-pages-that-werent-compacted.patch >>>> z3fold-fix-header-size-related-issues.patch >>>> z3fold-fix-locking-issues.patch >>> >>> My initial suggestion was to have it the following way: >>> z3fold-limit-first_num-to-the-actual-range-of-possible-buddy-indexes.patch >> >> this is a good one, acked by both of us; it should stay and go upstream to Linus >> >>> z3fold-make-pages_nr-atomic.patch >> >> the change itself looks ok and I acked it, but as Andrew commented the >> log says nothing about why it's being changed; the atomic function is >> slower so the log should explain why it's being changed; anyone >> reviewing the log history won't know why you made the change, and the >> change all by itself is a step backwards in performance. >> >>> z3fold-extend-compaction-function.patch >> >> this explictly has a bug in it that's fixed in one of the later >> patches; instead, this should be fixed up and resent. >> >>> z3fold-use-per-page-spinlock.patch >> >> i should have explicitly nak'ed this, as not only did it add a bug >> (fixed by the the other 'fix-' patch below) but its design should be >> replaced by kref counting, which your latest patch is working >> towards... >> >>> z3fold-fix-header-size-related-issues.patch >>> z3fold-fix-locking-issues.patch >> >> and these fix the known problems in the previous patches. >> >>> >>> I would prefer to keep the fix-XXX patches separate since e. g. >>> z3fold-fix-header-size-related-issues.patch concerns also the problems >>> that have been in the code for a while now. I am ok with folding these >>> into the relevant main patches but once again, given that some fixes >>> are related to the code that is already merged, I don't see why it >>> would be better. >> >> none of those patches are "merged", the last z3fold patch in Linus' >> tree is 43afc194 from June. Just because they're in Andrew's mmotm >> queue (and/or linux-next) doesn't mean they are going to be >> merged...(correct me please if I'm wrong there Andrew) > > that I do understand, however, > z3fold-fix-header-size-related-issues.patch fixes the off-by-one issue > present in the code that is in Linus's tree too. sorry, I just looked at this in mmotm and it does look good; I must have been confused because this was sent as part of a 2-patch series, but the two patches don't seem related :-) > >> So as you can see by my patch-by-patch breakdown, almost all of them >> need changes based on feedback from various people. And they are all >> related - your goal is to improve z3fold performance, right? IMHO >> they should be sent as a single patch series with that goal in the >> cover letter, including specific details and numbers about how the >> series does improve performance. > > but that is a good idea anyway, the only thing i\m not sure about is > whether it makes sense to fold > z3fold-fix-header-size-related-issues.patch into another or not. no that looks ok to separate, it's a standalone bugfix. I'm just saying, for the patches where problems were identified already, resend them with the patches fixed; and any that are related, send as a series. > > ~vitaly ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-12-23 13:10 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-26 19:15 [PATCH 0/2] z3fold fixes Vitaly Wool 2016-11-26 19:17 ` [PATCH 1/2] z3fold: fix header size related issues Vitaly Wool 2016-11-26 19:21 ` [PATCH 2/2] z3fold: fix locking issues Vitaly Wool 2016-11-29 22:33 ` [PATCH 0/2] z3fold fixes Dan Streetman 2016-11-29 22:39 ` Andrew Morton 2016-12-18 8:15 ` Vitaly Wool 2016-12-22 21:55 ` Dan Streetman 2016-12-22 23:04 ` Vitaly Wool 2016-12-23 13:09 ` Dan Streetman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).