From: Li Wang <liwang@redhat.com>
To: Gregory Price <gregory.price@memverge.com>
Cc: ltp@lists.linux.it, Gregory Price <gourry.memverge@gmail.com>
Subject: Re: [LTP] [PATCH] mempolicy/mbind: update syscall tests for weighted interleave
Date: Wed, 10 Apr 2024 16:44:43 +0800 [thread overview]
Message-ID: <CAEemH2fSFBw92+3jHuCsMSg0R+8JvPpQwv2xBE9NaWFaNrrCCw@mail.gmail.com> (raw)
In-Reply-To: <ZhXR7gdhKIFVEdvS@memverge.com>
Hi Gregory,
On Wed, Apr 10, 2024 at 7:40 AM Gregory Price <gregory.price@memverge.com>
wrote:
> On Mon, Apr 08, 2024 at 03:40:38PM +0800, Li Wang wrote:
> > Hi Gregory,
> >
> > Thank you for starting this, comments are inline below.
> >
> > > +#define MPOL_WEIGHTED_INTERLEAVE 6
> > > +#endif
> > >
> >
> > And can we move this common part into include/lapi/numaif.h,
> > to avoid defining this again and again in test cases?
> >
>
> I have a pending patch to do just that, but it is not upstream yet.
>
> This was a comment in the changelog:
>
> > > MPOL_WEIGHTED_INTERLEAVE is ifdef defined because it is not upstream
> > > in libnuma yet, so this ensures compilation.
>
> Thought it was useful to shoot out a version of this before it all lands
> for the sake of getting ahead of the curve a bit.
>
> >
> > First, we do not suggest adding any new tests by applying one "big"
> > patch especially since this contains too many other irrelevant
> > modifications.
> > We'd better separate them in single to guarantee everything goes
> > well for traceability of the commit.
> >
>
> Will do.
>
> > Second, I don't see any new code in set_mempolicy06/07, since you
> > only copied them from set_mempolicy02/04, even without any change of the
> > comments, this is bad for test maintenance work and involves redundant
> > stuff.
> >
>
> the only major differences between the tests, presently, are that the
> policy applied is weighted interleave
>
> TEST(set_mempolicy(MPOL_WEIGHTED_INTERLEAVE, bm->maskp, bm->size+1));
> ^^^^^^^^^^^^^^^^^^^^^^^^
>
> In truth, this test isn't really completely, as it should also:
>
> 1) Set the sysfs values located at
> /sys/kernel/mm/mempolicy/weighted_interleave/
>
> 2) Validate allocations actually match those settings
>
> However, this test is quite complicated to write and make fully
> reliable, as you also need to know
>
> 1) The environment (available nodes, cpu nodes, memory-only nodes)
> 2) The node the test will be run on (which can be forced)
> 3) Where allocations will start from (node X or node Y) as this can
> ultimately affect the final distribution.
>
> In my tests separately, the test itself can also cause allocation
> (stack, other regions) which may result in an unexpected distribution of
> memory on the target region. This is because those allocations are
> credited as part of the interleaving, but the existing code of the test
> cannot adjust for that. This may cause failures for no obvious reason.
>
> This is ultimately why I left the tests mostly unchanged, because I
> found it only reasonable to test the default behavior.
>
> > The recommended way is to add MPOL_WEIGHTED_INTERLEAVE in
> > the original case if you just want to validate the behavior similarly
> with
> > MPOL_INTERLEAVE.
> >
> > But if you want to test something special/new of
> MPOL_WEIGHTED_INTERLEAVE,
> > I think that's necessary to create set_mempolicy06/07, and do something
> > validate that interleaving behavior is executed based on weights set in '
> > /sys/kernel/mm/mempolicy/weighted_interleave/'.
> >
>
> Was hoping to get input on this. I think probably trying to write a
> test to track page-distribution of real weighted interleave will be
> difficult to make reliable, so maybe I should just fold these tests back
> into the original and note that this simply tests the default behavior
> (which is equivalent to MPOL_INTERLEAVE).
>
Ok, sure. If so merge the MPOL_WEIGHTED_INTERLEAVE into
MPOL_INTERLEAVE test should be enough.
>
> That may require changing the original tests to ensure the sysfs files
> are set to 1 to explicitly avoid failures. But I wasn't sure if that
> was ok since it would be making silent system-wide changes, and would
> require root.
>
LTP lib provides a way to save/restore the '/proc | /sys' value
before and after doing the test, so it would not be difficult to
satisfy the requirement if it is just set 1 to a relative file.
see:
https://github.com/linux-test-project/ltp/blob/master/doc/old/C-Test-API.asciidoc#127-saving--restoring-procsys-values
Or, use tst_sys_conf_save() separately to traverse and save the value of
"mempolicy/weighted_interleave/node*" file if we are unsure how many nodes
are on the system, then set the value via:
SAFE_FILE_PRINTF("../weighted_interleave/node*", "%d", 1);
Ultimately, LTP lib will restore all of the values to the original.
And set ".needs_root = 1," in the struct tst_test will be the requested
root permission.
--
Regards,
Li Wang
--
Mailing list info: https://lists.linux.it/listinfo/ltp
prev parent reply other threads:[~2024-04-10 8:45 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-29 18:07 [LTP] [PATCH] mempolicy/mbind: update syscall tests for weighted interleave Gregory Price
2024-04-08 7:40 ` Li Wang
2024-04-09 23:40 ` Gregory Price
2024-04-10 8:44 ` Li Wang [this message]
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=CAEemH2fSFBw92+3jHuCsMSg0R+8JvPpQwv2xBE9NaWFaNrrCCw@mail.gmail.com \
--to=liwang@redhat.com \
--cc=gourry.memverge@gmail.com \
--cc=gregory.price@memverge.com \
--cc=ltp@lists.linux.it \
/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;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).