num_numa domains

[ron minnich]

In the function num_numa domains, it sets the number to -1 and increments it once for each domain it finds. 

IMHO, the number of numa domains in the base case should be one, not 0. the code certainly goes to pieces if it is set to 0. Thoughts?

ron

[Barret Rhoden]

/* Figure out the maximum number of numa domains we actually have and set it in
* our cpu_topology_info struct. */
static void set_num_numa(void)
{
num_numa = -1;
struct Srat *temp = srat;
while (temp) {
if (temp->type == SRlapic)
if (temp->lapic.dom > num_numa)
num_numa++;
temp = temp->next;
}
num_numa++;
}

Yeah, perhaps having num_numa = 0, drop the ++ at the end, and have a

num_numa = MIN(1, num_numa);

to enforce the min?

Barret

[Kevin Klues]

It's written as it is so that

if (temp->lapic.dom > num_numa)
num_numa++;

will increment the domain number, even if the lowest domain it finds
has an id of 0.

> --
> You received this message because you are subscribed to the Google Groups "Akaros" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to akaros+un...@googlegroups.com.
> To post to this group, send email to aka...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.



--
~Kevin

[ron minnich]

But it has to change :-)

ron

[Kevin Klues]

That's fine, I'm just trying to understand why before we do it.

[ron minnich]

so here's an option. We could call the numa code and, if it returns 0, just do a flat topology. 

Just extend the check a litlte bit: if cpu_bits && num_numa > 0

ron

[Kevin Klues]

That seems reasonable to me. That seems like more of a fallback though
for when the acpi tables can't be parsed for whatever reason. I'm
still unclear why it's not working in the current setup if your acpi
tables are good.

[Barret Rhoden]

On 2015-10-05 at 10:07 Kevin Klues <klu...@gmail.com> wrote:
> That seems reasonable to me. That seems like more of a fallback though
> for when the acpi tables can't be parsed for whatever reason. I'm
> still unclear why it's not working in the current setup if your acpi
> tables are good.

Perhaps an alternative solution is to push a fix into the ACPI code.
If the ACPI parsing comes up with no numa domains, we fix up our ACPI
structs to have at least one. We can put in similar checks for other
invariants.

Barret

[ron minnich]

The ACPI tables are good. They don't contain all the things you want to see in them. This is allowed.

The things you're looking for in the ACPI table don't have to be there. I learned this the hard way over the years and

had to put fixes into the Plan 9 parser to cover those cases. I just love ACPI.

I don't think we should mess with the ACPI tables, personally. Start down that road, it will never end, and we'll have code in different places trying to change things for different reasons. 

We're a consumer of ACPI tables. Check out the fixnuma branch.

It's a trivial change.

ron

[Kevin Klues]

The change seems ok, but the downside now is that you don't get the
rest of the topology information (which comes from cpuid, not acpi),
just because your numa stuff wasn't present in the ACPI tables. I
think I'd rather just default to 1 in set_num_numa() (using Barret's
suggested method of returning num_numa = MIN(1, num_numa)) since there
obviously is at least 1 numa domain. You will lose the ability to
distinguish numa domains, but you will still be able to detect cpu and
socket boundaries.

Is the problem that the coreboot APCI tables just don't have numa
information in them at all?

[ron minnich]

it breaks still if you just set numa to 1 (first thing I tried). That's not sufficient: it still explodes. 

If you go that route, the changes need to go further. I didn't want to go after everything and, besides,

it's very likely on a system without numa info that it's a single socket, and hence there's no real numa-ness to discover.

There's no numa info in coreboot acpi tables I suspect because there have been no numa intel systems ever

(not the case for AMD, just intel. Thank you, Intel). Since it's optional, well, I assume it was left out.

But the code should tolerate what we consider incomplete acpi tables. That's just the way it is with ACPI: lots of broken

and incomplete tables, and it's up to the kernel to be forgiving.

ron

[Kevin Klues]

Fair enough. Let's go with the flat topology fallback then.

[Barret Rhoden]

On 2015-10-05 at 17:18 ron minnich <rmin...@gmail.com> wrote:
> The ACPI tables are good. They don't contain all the things you want
> to see in them. This is allowed.
>
> The things you're looking for in the ACPI table don't have to be
> there. I learned this the hard way over the years and
> had to put fixes into the Plan 9 parser to cover those cases. I just
> love ACPI.
>
> I don't think we should mess with the ACPI tables, personally. Start
> down that road, it will never end, and we'll have code in different
> places trying to change things for different reasons.

Just to be clear, I didn't mean to mess with the ACPI tables. I meant
to mess with our in-kernel data structures that we built from parsing
ACPI. And that fiddling happens in the ACPI parsing code itself, such
that we can express and enforce a few invariants on the kernel data
structures.

For instance, we can mandate that there is at least one lapic and it's
domain is >= 0 (if that makes sense). We can add something like
acpi_check() at the end of acpiinit() that will:

struct Srat *temp = srat;
while (temp)

if ((temp->type == SRlapic) && (temp->lapic.dom >= 0))
break;
if (!temp)
panic("") || return -1 || whatever();

And in the case of the chromebook, we now know that we're going to fail
that check, so we'll need to catch it and patch up the struct *srat srat
list by creating a new struct Srat * and adding it to the list. This
way, we localize the fix (kernel structures do Foo) closest to the
problem (ACPI tables).

Anyway, I'm up for whatever fix you guys have.

Barret

[ron minnich]

I understood what you were saying, but still feel the in-memory ACPI tables should be as faiithful a representation as

possible to the firmware. I'm uncomfortable with that level of tweaking. I think it will bite us some other way -- even 

bite more than ACPI does already.

OK if we go with what I posted and maybe revisit later if it comes up in a different way? in this case I feel 

that flat model makes the most sense anyway.

ron

[Barret Rhoden]

On 2015-10-06 at 02:36 ron minnich <rmin...@gmail.com> wrote:
> I understood what you were saying, but still feel the in-memory ACPI
> tables should be as faiithful a representation as
> possible to the firmware. I'm uncomfortable with that level of
> tweaking. I think it will bite us some other way -- even
> bite more than ACPI does already.
>
> OK if we go with what I posted and maybe revisit later if it comes up
> in a different way? in this case I feel
> that flat model makes the most sense anyway.

Sounds good. We'll see what comes up next in the ACPI house of horrors.


On a related note:

static int get_num_numa(void)
{
int numa = -1;

struct Srat *temp = srat;
while (temp) {

if (temp->type == SRlapic)

if (temp->lapic.dom > numa)
numa++;
temp = temp->next;
}
return numa + 1;
}

Does this loop assume that the Srat list has the SRlapics
sorted, such that lapic.dom increase monotonically?

Barret

[Kevin Klues]

Yeah, something seems off about that loop. If they are not sorted and
monotonically increasing, we will miss some. What we really want is
to go through the loop and count the number of unique
'temp->lapic.dom' instances we find.

> --
> You received this message because you are subscribed to the Google Groups "Akaros" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to akaros+un...@googlegroups.com.
> To post to this group, send email to aka...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

--
~Kevin

[Kevin Klues]

I have a fix, will test in the morning.

--
~Kevin

[Kevin Klues]

Resolved by:

https://github.com/brho/akaros/commit/f0f3cad78ccaf927c426bb7fac5d434f676393ea

https://groups.google.com/d/msg/akaros/9OcHACel5_g/MP80-EALCQAJ