On Thu, Jun 05, 2025 at 05:42:55PM +0100, Mark Brown wrote:
> On Thu, Jun 05, 2025 at 05:26:05PM +0100, Lorenzo Stoakes wrote:
> > On Thu, Jun 05, 2025 at 05:15:51PM +0100, Mark Brown wrote:
>
> > > That's the thing with memfd being special and skipping on setup failure
> > > that David mentioned, I've got a patch as part of the formatting series
> > > I was going to send after the merge window.
>
> > where did he mention this?
>
> I can't remember off hand, sorry.
>
I see his reply here and I guess it's because you now determine the result at
the top level, I see you are doing this in do_test():
+ int result = KSFT_PASS;
int ret;
+ if (fd < 0) {
+ result = KSFT_FAIL;
+ goto report;
+ }
+
And in run_with_memfd_hugetlb():
fd = memfd_create("test", flags);
if (fd < 0) {
- ksft_test_result_skip("memfd_create() failed (%s)\n",
strerror(errno));
- return;
+ ksft_print_msg("memfd_create() failed (%s)\n", strerror(errno));
}
So previously this would skip, now it fails.
I've not double, triple checked this is it, but seems likely!
You said you had a fix
> > I mean I'd argue that making a test that previously worked now fail due to
> > how
> > somebody's set up their system is a reason not to merge that patch.
>
> Well, it's a bit late now given that this is in Linus' tree and actually
> it turns out this was the only update for gup_longterm so I just rebased
> it onto Linus' tree and kicked off my tests.
Ack.
>
> > Better to do all of these formating fixes and maintain the _same behaviour_
> > then
> > separately tackle whether or not we should skip.
>
> I'm confused, that's generally the opposite of the standard advice for
> the kernel - usually it's fixes first, then deal with anything cosmetic
> or new?
I mean the crux is that the 'cosmetic' changes also included a 'this might
break things' change.
I'm saying do the cosmetic things in _isolation_, or fix the brokenness
before doing the whole lot.
Anyway there's no point dwelling on this, let's just get a fix sorted.
>
> > Obviously the better option would be to somehow determine if hugetlb is
> > available in advance (of course, theoretically somebody could come in and
> > reserve pages but that's not veyr likely).
>
> The tests do enumerate the set of available hugepage sizes at runtime
> (see the loop in run_test_case()) but detect_hugetlb_page_sizes() just
> looks in /sys/kernel/mm/hugepages/ for subdirectories and doesn't look
> inside those directories to see if there are actually any huge pages
> available for the huge page sizes advertised. There's probably utility
> in at least a version of that function that checks.
Right yes, I mean obviously this whole thing is a mess already that's not
your fault, and ideally we'd have some general way of looking this up
across _all_ tests and just switch things on/off accordingly.
There's a whole Pandora's box about what the tests should assume/not and
yeah. Anyway. Maybe leave it closed for now :)
Cheers, Lorenzo
