linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: "Liam R. Howlett" <Liam.Howlett@oracle.com>
To: Wei Yang <richard.weiyang@gmail.com>
Cc: akpm@linux-foundation.org, maple-tree@lists.infradead.org,
	linux-mm@kvack.org, Sidhartha Kumar <sidhartha.kumar@oracle.com>,
	Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Subject: Re: [PATCH v2 1/2] maple_tree: simplify split calculation
Date: Tue, 12 Nov 2024 20:44:36 -0500	[thread overview]
Message-ID: <k34yl5ikvujh6n2yzkj6e3qwhdpz4cfwadjla6yw76emuydunn@3g3nxvwtnble> (raw)
In-Reply-To: <20241113011542.lw5zzude7oo63rr7@master>

* Wei Yang <richard.weiyang@gmail.com> [241112 20:15]:
> On Tue, Nov 12, 2024 at 09:46:20AM -0500, Liam R. Howlett wrote:
> >* Wei Yang <richard.weiyang@gmail.com> [241109 08:45]:
> >> We have been too smart to calculate split value.
> >> 
> >> The purpose of current calculation is to avoid having a range less than
> >> the slot count. But this seems to push too hard to suffer from jitter
> >> problem.
> >> 
> >> Considering this only matters if the range is less than the slot count,
> >> so the real world implications of the calculation will be negligible. So
> >> we decide to simplify the calculation of split.
> >> 
> >> Also current code may lead to deficient node, the condition to check
> >> should be (b_end - split - 1 > slot_min). After this change, this one is
> >> gone together.
> >
> >This comment is difficult to understand.
> >
> >Maybe something like:
> >The current calculation for splitting nodes tries to enforce a minimum
> >span on the leaf nodes.  This code is complex and never worked correctly
> >to begin with, due to the min value being passed as 0 for all leaves.
> >
> >The calculation should just split the data as equally as possible
> >between the new nodes.  Note that b_end will be one more than the data,
> >so the left side is still favoured in the calculation.
> >
> >The current code may also lead to a deficient node by not leaving enough
> >data for the right side of the split. This issue is also addressed with
> >the split calculation change.
> >
> 
> Thanks, this looks much better :-)
> 
> >> 
> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> >
> >Fixes: ?
> >Cc: stable ?
> >
> 
> Will add this.
> 
> BTW, as this is a fix, do you think the test case in patch 2 of v1 is still
> necessary?

Yes, please include it.

I keep all test cases from fixed bugs or added features.  This way we
know that the bug doesn't return in another unrelated change.  The
testcases continuously grow, but they are always useful.

Thanks,
Liam


  reply	other threads:[~2024-11-13  1:44 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-11-09 13:44 [PATCH v2 0/2] " Wei Yang
2024-11-09 13:44 ` [PATCH v2 1/2] maple_tree: " Wei Yang
2024-11-12 14:46   ` Liam R. Howlett
2024-11-13  1:15     ` Wei Yang
2024-11-13  1:44       ` Liam R. Howlett [this message]
2024-11-09 13:44 ` [PATCH v2 2/2] maple_tree: only root node could be deficient Wei Yang
2024-11-12 14:46   ` Liam R. Howlett

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=k34yl5ikvujh6n2yzkj6e3qwhdpz4cfwadjla6yw76emuydunn@3g3nxvwtnble \
    --to=liam.howlett@oracle.com \
    --cc=akpm@linux-foundation.org \
    --cc=linux-mm@kvack.org \
    --cc=lorenzo.stoakes@oracle.com \
    --cc=maple-tree@lists.infradead.org \
    --cc=richard.weiyang@gmail.com \
    --cc=sidhartha.kumar@oracle.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox