Hi Dave,
Am Dienstag, den 28.04.2009, 14:24 -0400 schrieb Dave Anderson:
>
> Michael,
>
> It's not clear to me how this function works:
>
> + * Get cpu map (possible, online, etc.) address from cpu mask (since 2.6.29)
> + */
> +static ulong
> +get_cpu_map_addr_from_mask(const char *type)
> +{
> + ulong cpu_mask_addr, cpu_map_addr;
> + char cpu_mask_symbol[32];
> + char *cpu_mask_buf;
> + int cpu_mask_size;
> +
> + sprintf(cpu_mask_symbol, "cpu_%s_mask", type);
> +
> + if (!symbol_exists(cpu_mask_symbol))
> + return 0;
> +
> + cpu_mask_addr = symbol_value(cpu_mask_symbol);
> + cpu_mask_size = STRUCT_SIZE("cpumask");
> + cpu_mask_buf = malloc(cpu_mask_size);
> + if (!cpu_mask_buf)
> + error(FATAL, "get_cpu_map_addr_from_mask: out of memory\n");
> + readmem(cpu_mask_addr, KVADDR, cpu_mask_buf, cpu_mask_size,
> + cpu_mask_symbol, FAULT_ON_ERROR);
> + cpu_map_addr = ULONG(cpu_mask_buf + MEMBER_OFFSET("cpumask", "bits"));
> + free(cpu_mask_buf);
> +
> + return cpu_map_addr;
> +}
>
> The caller to this function expects the address of a cpumask_t
> data structure. As I understand it, both before and after 2.6.29,
> the cpumask data structure looks like:
>
> typedef struct cpumask {
> unsigned long int bits[<variable-based-upon-NR_CPUS>];
> } cpumask_t;
>
> And pre-2.6.29, the 3 "cpu_xxxx_map" symbols were all statically
> defined cpumask structures:
>
> cpumask_t cpu_online_mask;
> cpumask_t cpu_possible_mask;
> cpumask_t cpu_present_mask;
>
> Whereas now the 3 symbols don't exist except as #define's like so:
>
> #define cpu_possible_map (*(cpumask_t *)cpu_possible_mask)
> #define cpu_online_map (*(cpumask_t *)cpu_online_mask)
> #define cpu_present_map (*(cpumask_t *)cpu_present_mask)
>
> and -- if I understand the code correctly -- each cpu_xxxx_mask
> is just a pointer to a cpumask data structure.
>
> So when the caller needs the address of the relevant cpumask data structure
> why doesn't your cpu_map_addr_from_mask() function simply do something
> like:
>
> get_symbol_data(cpu_mask_symbol, sizeof(void *), &tmp);
> return tmp;
>
> i.e., which would read the contents of the cpu_mask_symbol, which
> is the pointer that the caller wants.
>
> Instead, your function reads in the whole data structure, where
> the cpumask.bits[] array typically contains several items, i.e.,
> enough longs to contain NR_CPUS bits. Then, with ULONG(), it is
> essentially reading the unsigned long value found in the first
> bit[] array item, or:
>
> cpumask.bits[0];
>
> and therefore returning the *bitmask* contained in the first unsigned
> long in the bit[] array -- instead of the *address* of the cpumask
> data structure. How can that work?
>
> Or am I missing something?
No, you are completely right.
I tested my patch and it worked. But it only worked by chance. I read
the address using readmem(cpu_mask_size) in combination with ULONG(),
which is nonsense.
So the correct (and tested) version would be something like:
static ulong
get_cpu_map_addr_from_mask(const char *type)
{
char cpu_mask_symbol[32];
ulong cpu_map_addr;
sprintf(cpu_mask_symbol, "cpu_%s_mask", type);
if (!symbol_exists(cpu_mask_symbol))
return 0;
get_symbol_data(cpu_mask_symbol, sizeof(ulong), &cpu_map_addr);
return cpu_map_addr;
}
Michael
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
