Page MenuHomeFreeBSD

powerpc: unconditionally mark SLB zones UMA_ZONE_CONTIG
ClosedPublic

Authored by rlibby on Feb 17 2020, 6:28 PM.

Details

Summary

PR: 244118
Reported by: Francis Little
Tested by: Francis Little, Mark Millard

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

rlibby created this revision.Feb 17 2020, 6:28 PM
markj added a comment.Feb 17 2020, 6:36 PM

Can you explain why this makes a difference, at least for slbt_zone? I think I am missing something.

markmi_dsl-only.net added a comment.EditedFeb 17 2020, 7:10 PM

Can you explain why this makes a difference, at least for slbt_zone? I think I am missing something.

I can report observed behavior both ways:

Without removing the conditional structure the old PowerMac G5 "Quads"
do not boot. With the removal they do. The booting has been broken since
head -r357549 when the conditional allocf_flags code structure was put in.

See https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=244118 .

I'm now running with the patch from that bugzilla and have progressed to
-r357979 with it.

Can you explain why this makes a difference, at least for slbt_zone? I think I am missing something.

To be honest, I'm not sure that it does matter for slbt_zone, and my reasoning was just that we already use the contig allocator for the platform_real_maxaddr() != VM_MAX_ADDRESS case, so perhaps it matters? I'm not sure if that is purposeful or if it may be partially an accident from wanting to use the maxaddr constraint for that zone.

The slbt_zone has slbtnode objects, which seem to contain embedded slb objects (slb_entries). I think but I'm not sure that this object is where the constraints are coming from.

In practice this doesn't have an effect because the zone already met the minimum efficiency and I think because the object size made it so that the struct slbs wouldn't break across pages already.

If there's someone who knows this area of code better and can describe the actual constraints, we can try to tune it more finely. I think if it's overconstrained, then it's at least safe.

For slb_cache_zone, I think the relevant code is moea64_activate().

Can you explain why this makes a difference, at least for slbt_zone? I think I am missing something.

I can report observed behavior both ways:

Without removing the conditional structure the old PowerMac G5 "Quads"
do not boot. With the removal they do. The booting has been broken since
head -r357549 when the conditional allocf_flags code structure was put in.

See https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=244118 .

I'm now running with the patch from that bugzilla and have progressed to
-r357979 with it.

So, my understanding is that the change for slb_cache_zone is necessary (demonstrated by testing), and whether the change is necessary (in principle) for slbt_zone is not clear.

The issue in general is whether it is okay for objects from these zones to be backed by physical memory that is not contiguous, which UMA multipage slabs may cause unless the UMA_ZONE_CONTIG flag is specified or uma_zone_set_allocf is used.

Note that this is not a revert of the change in r357548, this is adding the UMA_ZONE_CONTIG flag unconditionally.

markj accepted this revision.Feb 17 2020, 9:07 PM

The issue in general is whether it is okay for objects from these zones to be backed by physical memory that is not contiguous, which UMA multipage slabs may cause unless the UMA_ZONE_CONTIG flag is specified or uma_zone_set_allocf is used.

Note that this is not a revert of the change in r357548, this is adding the UMA_ZONE_CONTIG flag unconditionally.

Ah, that clarifies things. I was looking for a problem in r357548.

This revision is now accepted and ready to land.Feb 17 2020, 9:07 PM

. . .> Ah, that clarifies things. I was looking for a problem in r357548.

FYI: My testing included a -r357548 kernel on the G5 Quad and it worked fine.
It was specifically -r357549 where things quit working.

This revision was automatically updated to reflect the committed changes.