4:03 a.m.
I've been wondering about the intended use of this facilitator macro
for some time and concluded that it is plainly wrong. It should
take a pointer value from a buffer, but what is the use of a pointer
that pointed to something on the target machine?
Since it cannot be meaningfully dereferenced on the host, the only
(arguable) advantage is the ability to do pointer arithmetics on the
variables. This second patch does the correct thing wherever pointer
arithmetics was done, making the actual calculations more explicit.
Signed-off-by: Petr Tesarik <ptesarik(a)suse.cz>
---
memory.c | 47 +++++++++++++++++++++++++----------------------
1 file changed, 25 insertions(+), 22 deletions(-)
--- a/memory.c
+++ b/memory.c
@@ -26,8 +26,8 @@ struct meminfo { /* general pu
ulong c_offset;
ulong c_num;
ulong s_mem;
- void *s_freep;
- ulong *s_index;
+ ulong s_freep;
+ ulong s_index;
ulong s_inuse;
ulong cpucached_cache;
ulong cpucached_slab;
@@ -139,7 +139,7 @@ static int next_kpage(ulong, ulong *);
static ulong last_vmalloc_address(void);
static ulong next_vmlist_vaddr(ulong);
static int next_identity_mapping(ulong, ulong *);
-static int vm_area_page_dump(ulong, ulong, ulong, ulong, void *,
+static int vm_area_page_dump(ulong, ulong, ulong, ulong, ulong,
struct reference *);
static int dump_swap_info(ulong, ulong *, ulong *);
static void swap_info_init(void);
@@ -3138,7 +3138,7 @@ vm_area_dump(ulong task, ulong flag, ulo
ulong vma;
ulong vm_start;
ulong vm_end;
- void *vm_next, *vm_mm;
+ ulong vm_next, vm_mm;
char *dentry_buf, *vma_buf, *file_buf;
ulong vm_flags;
ulong vm_file, inode;
@@ -3202,7 +3202,7 @@ vm_area_dump(ulong task, ulong flag, ulo
!DO_REF_SEARCH(ref))
fprintf(fp, vma_header);
- for (found = FALSE; vma; vma = (ulong)vm_next) {
+ for (found = FALSE; vma; vma = vm_next) {
if ((flag & PHYSADDR) && !DO_REF_SEARCH(ref))
fprintf(fp, "%s", vma_header);
@@ -3211,9 +3211,9 @@ vm_area_dump(ulong task, ulong flag, ulo
BZERO(buf1, BUFSIZE);
vma_buf = fill_vma_cache(vma);
- vm_mm = VOID_PTR(vma_buf + OFFSET(vm_area_struct_vm_mm));
+ vm_mm = ULONG(vma_buf + OFFSET(vm_area_struct_vm_mm));
vm_end = ULONG(vma_buf + OFFSET(vm_area_struct_vm_end));
- vm_next = VOID_PTR(vma_buf + OFFSET(vm_area_struct_vm_next));
+ vm_next = ULONG(vma_buf + OFFSET(vm_area_struct_vm_next));
vm_start = ULONG(vma_buf + OFFSET(vm_area_struct_vm_start));
vm_flags = SIZE(vm_area_struct_vm_flags) == sizeof(short) ?
USHORT(vma_buf+ OFFSET(vm_area_struct_vm_flags)) :
@@ -3335,7 +3335,7 @@ vm_area_page_dump(ulong vma,
ulong task,
ulong start,
ulong end,
- void *mm,
+ ulong mm,
struct reference *ref)
{
physaddr_t paddr;
@@ -3347,7 +3347,7 @@ vm_area_page_dump(ulong vma,
char buf3[BUFSIZE];
char buf4[BUFSIZE];
- if ((ulong)mm == symbol_value("init_mm"))
+ if (mm == symbol_value("init_mm"))
return FALSE;
if (!ref || DO_REF_DISPLAY(ref))
@@ -9720,9 +9720,9 @@ dump_slab(struct meminfo *si)
return;
}
- si->s_freep = VOID_PTR(si->slab_buf + OFFSET(kmem_slab_s_s_freep));
+ si->s_freep = ULONG(si->slab_buf + OFFSET(kmem_slab_s_s_freep));
si->s_inuse = ULONG(si->slab_buf + OFFSET(kmem_slab_s_s_inuse));
- si->s_index = ULONG_PTR(si->slab_buf + OFFSET(kmem_slab_s_s_index));
+ si->s_index = ULONG(si->slab_buf + OFFSET(kmem_slab_s_s_index));
s_offset = USHORT(si->slab_buf + OFFSET(kmem_slab_s_s_offset));
if (!(si->flags & ADDRESS_SPECIFIED)) {
@@ -9850,7 +9850,7 @@ dump_slab_percpu_v2(struct meminfo *si)
static void
gather_slab_free_list(struct meminfo *si)
{
- ulong *next, obj;
+ ulong next, obj;
ulong expected, cnt;
BNEG(si->addrlist, sizeof(ulong) * (si->c_num+1));
@@ -9885,16 +9885,18 @@ gather_slab_free_list(struct meminfo *si
*/
if (si->c_flags & SLAB_CFLGS_BUFCTL)
- obj = si->s_mem + ((next - si->s_index) * si->c_offset);
+ obj = si->s_mem +
+ ((next - si->s_index) / sizeof(ulong)
+ * si->c_offset);
else
- obj = (ulong)next - si->c_offset;
+ obj = next - si->c_offset;
si->addrlist[cnt] = obj;
if (si->flags & ADDRESS_SPECIFIED) {
if (INSLAB(next, si) &&
- (si->spec_addr >= (ulong)next) &&
- (si->spec_addr < (ulong)(next + 1))) {
+ (si->spec_addr >= next) &&
+ (si->spec_addr < next + sizeof(ulong))) {
si->found = KMEM_BUFCTL_ADDR;
return;
}
@@ -9909,7 +9911,7 @@ gather_slab_free_list(struct meminfo *si
si->errors++;
}
- readmem((ulong)next, KVADDR, &next, sizeof(void *),
+ readmem(next, KVADDR, &next, sizeof(ulong),
"s_freep chain entry", FAULT_ON_ERROR);
} while (next);
@@ -10057,7 +10059,7 @@ static void
dump_slab_objects(struct meminfo *si)
{
int i, j;
- ulong *next;
+ ulong next;
int on_free_list;
ulong cnt, expected;
ulong bufctl, obj;
@@ -10088,7 +10090,8 @@ dump_slab_objects(struct meminfo *si)
if (si->c_flags & SLAB_CFLGS_BUFCTL) {
for (i = 0, next = si->s_index; i < si->c_num; i++, next++) {
obj = si->s_mem +
- ((next - si->s_index) * si->c_offset);
+ ((next - si->s_index) / sizeof(ulong)
+ * si->c_offset);
DUMP_SLAB_OBJECT();
}
} else {
@@ -11561,7 +11564,7 @@ next_upage(struct task_context *tc, ulon
int found;
char *vma_buf;
ulong vm_start, vm_end;
- void *vm_next;
+ ulong vm_next;
if (!tc->mm_struct)
return FALSE;
@@ -11575,12 +11578,12 @@ next_upage(struct task_context *tc, ulon
vaddr = VIRTPAGEBASE(vaddr) + PAGESIZE(); /* first possible page */
- for (found = FALSE; vma; vma = (ulong)vm_next) {
+ for (found = FALSE; vma; vma = vm_next) {
vma_buf = fill_vma_cache(vma);
vm_start = ULONG(vma_buf + OFFSET(vm_area_struct_vm_start));
vm_end = ULONG(vma_buf + OFFSET(vm_area_struct_vm_end));
- vm_next = VOID_PTR(vma_buf + OFFSET(vm_area_struct_vm_next));
+ vm_next = ULONG(vma_buf + OFFSET(vm_area_struct_vm_next));
if (vaddr <= vm_start) {
*nextvaddr = vm_start;
8:08 a.m.
----- Original Message -----
I've been wondering about the intended use of this facilitator
macro
for some time and concluded that it is plainly wrong. It should
take a pointer value from a buffer, but what is the use of a pointer
that pointed to something on the target machine?
Since it cannot be meaningfully dereferenced on the host, the only
(arguable) advantage is the ability to do pointer arithmetics on the
variables. This second patch does the correct thing wherever pointer
arithmetics was done, making the actual calculations more explicit.
Signed-off-by: Petr Tesarik <ptesarik(a)suse.cz>
Everything *looks* correct -- but this patch doesn't fix anything, while
having the capacity to break what works. The slab-related changes still
concern me, at least until I test them.
BTW, in the future, when you post patches can you add them as attachments
instead of inlining them?
Thanks,
Dave
---
memory.c | 47 +++++++++++++++++++++++++----------------------
1 file changed, 25 insertions(+), 22 deletions(-)
--- a/memory.c
+++ b/memory.c
@@ -26,8 +26,8 @@ struct meminfo { /* general pu
ulong c_offset;
ulong c_num;
ulong s_mem;
- void *s_freep;
- ulong *s_index;
+ ulong s_freep;
+ ulong s_index;
ulong s_inuse;
ulong cpucached_cache;
ulong cpucached_slab;
@@ -139,7 +139,7 @@ static int next_kpage(ulong, ulong *);
static ulong last_vmalloc_address(void);
static ulong next_vmlist_vaddr(ulong);
static int next_identity_mapping(ulong, ulong *);
-static int vm_area_page_dump(ulong, ulong, ulong, ulong, void *,
+static int vm_area_page_dump(ulong, ulong, ulong, ulong, ulong,
struct reference *);
static int dump_swap_info(ulong, ulong *, ulong *);
static void swap_info_init(void);
@@ -3138,7 +3138,7 @@ vm_area_dump(ulong task, ulong flag, ulo
ulong vma;
ulong vm_start;
ulong vm_end;
- void *vm_next, *vm_mm;
+ ulong vm_next, vm_mm;
char *dentry_buf, *vma_buf, *file_buf;
ulong vm_flags;
ulong vm_file, inode;
@@ -3202,7 +3202,7 @@ vm_area_dump(ulong task, ulong flag, ulo
!DO_REF_SEARCH(ref))
fprintf(fp, vma_header);
- for (found = FALSE; vma; vma = (ulong)vm_next) {
+ for (found = FALSE; vma; vma = vm_next) {
if ((flag & PHYSADDR) && !DO_REF_SEARCH(ref))
fprintf(fp, "%s", vma_header);
@@ -3211,9 +3211,9 @@ vm_area_dump(ulong task, ulong flag, ulo
BZERO(buf1, BUFSIZE);
vma_buf = fill_vma_cache(vma);
- vm_mm = VOID_PTR(vma_buf + OFFSET(vm_area_struct_vm_mm));
+ vm_mm = ULONG(vma_buf + OFFSET(vm_area_struct_vm_mm));
vm_end = ULONG(vma_buf + OFFSET(vm_area_struct_vm_end));
- vm_next = VOID_PTR(vma_buf + OFFSET(vm_area_struct_vm_next));
+ vm_next = ULONG(vma_buf + OFFSET(vm_area_struct_vm_next));
vm_start = ULONG(vma_buf + OFFSET(vm_area_struct_vm_start));
vm_flags = SIZE(vm_area_struct_vm_flags) == sizeof(short) ?
USHORT(vma_buf+ OFFSET(vm_area_struct_vm_flags)) :
@@ -3335,7 +3335,7 @@ vm_area_page_dump(ulong vma,
ulong task,
ulong start,
ulong end,
- void *mm,
+ ulong mm,
struct reference *ref)
{
physaddr_t paddr;
@@ -3347,7 +3347,7 @@ vm_area_page_dump(ulong vma,
char buf3[BUFSIZE];
char buf4[BUFSIZE];
- if ((ulong)mm == symbol_value("init_mm"))
+ if (mm == symbol_value("init_mm"))
return FALSE;
if (!ref || DO_REF_DISPLAY(ref))
@@ -9720,9 +9720,9 @@ dump_slab(struct meminfo *si)
return;
}
- si->s_freep = VOID_PTR(si->slab_buf + OFFSET(kmem_slab_s_s_freep));
+ si->s_freep = ULONG(si->slab_buf + OFFSET(kmem_slab_s_s_freep));
si->s_inuse = ULONG(si->slab_buf + OFFSET(kmem_slab_s_s_inuse));
- si->s_index = ULONG_PTR(si->slab_buf + OFFSET(kmem_slab_s_s_index));
+ si->s_index = ULONG(si->slab_buf + OFFSET(kmem_slab_s_s_index));
s_offset = USHORT(si->slab_buf + OFFSET(kmem_slab_s_s_offset));
if (!(si->flags & ADDRESS_SPECIFIED)) {
@@ -9850,7 +9850,7 @@ dump_slab_percpu_v2(struct meminfo *si)
static void
gather_slab_free_list(struct meminfo *si)
{
- ulong *next, obj;
+ ulong next, obj;
ulong expected, cnt;
BNEG(si->addrlist, sizeof(ulong) * (si->c_num+1));
@@ -9885,16 +9885,18 @@ gather_slab_free_list(struct meminfo *si
*/
if (si->c_flags & SLAB_CFLGS_BUFCTL)
- obj = si->s_mem + ((next - si->s_index) * si->c_offset);
+ obj = si->s_mem +
+ ((next - si->s_index) / sizeof(ulong)
+ * si->c_offset);
else
- obj = (ulong)next - si->c_offset;
+ obj = next - si->c_offset;
si->addrlist[cnt] = obj;
if (si->flags & ADDRESS_SPECIFIED) {
if (INSLAB(next, si) &&
- (si->spec_addr >= (ulong)next) &&
- (si->spec_addr < (ulong)(next + 1))) {
+ (si->spec_addr >= next) &&
+ (si->spec_addr < next + sizeof(ulong))) {
si->found = KMEM_BUFCTL_ADDR;
return;
}
@@ -9909,7 +9911,7 @@ gather_slab_free_list(struct meminfo *si
si->errors++;
}
- readmem((ulong)next, KVADDR, &next, sizeof(void *),
+ readmem(next, KVADDR, &next, sizeof(ulong),
"s_freep chain entry", FAULT_ON_ERROR);
} while (next);
@@ -10057,7 +10059,7 @@ static void
dump_slab_objects(struct meminfo *si)
{
int i, j;
- ulong *next;
+ ulong next;
int on_free_list;
ulong cnt, expected;
ulong bufctl, obj;
@@ -10088,7 +10090,8 @@ dump_slab_objects(struct meminfo *si)
if (si->c_flags & SLAB_CFLGS_BUFCTL) {
for (i = 0, next = si->s_index; i < si->c_num; i++, next++) {
obj = si->s_mem +
- ((next - si->s_index) * si->c_offset);
+ ((next - si->s_index) / sizeof(ulong)
+ * si->c_offset);
DUMP_SLAB_OBJECT();
}
} else {
@@ -11561,7 +11564,7 @@ next_upage(struct task_context *tc, ulon
int found;
char *vma_buf;
ulong vm_start, vm_end;
- void *vm_next;
+ ulong vm_next;
if (!tc->mm_struct)
return FALSE;
@@ -11575,12 +11578,12 @@ next_upage(struct task_context *tc, ulon
vaddr = VIRTPAGEBASE(vaddr) + PAGESIZE(); /* first possible page */
- for (found = FALSE; vma; vma = (ulong)vm_next) {
+ for (found = FALSE; vma; vma = vm_next) {
vma_buf = fill_vma_cache(vma);
vm_start = ULONG(vma_buf + OFFSET(vm_area_struct_vm_start));
vm_end = ULONG(vma_buf + OFFSET(vm_area_struct_vm_end));
- vm_next = VOID_PTR(vma_buf + OFFSET(vm_area_struct_vm_next));
+ vm_next = ULONG(vma_buf + OFFSET(vm_area_struct_vm_next));
if (vaddr <= vm_start) {
*nextvaddr = vm_start;
--
Crash-utility mailing list
Crash-utility(a)redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
3:10 p.m.
----- Original Message -----
----- Original Message -----
> I've been wondering about the intended use of this facilitator macro
> for some time and concluded that it is plainly wrong. It should
> take a pointer value from a buffer, but what is the use of a pointer
> that pointed to something on the target machine?
>
> Since it cannot be meaningfully dereferenced on the host, the only
> (arguable) advantage is the ability to do pointer arithmetics on the
> variables. This second patch does the correct thing wherever pointer
> arithmetics was done, making the actual calculations more explicit.
>
> Signed-off-by: Petr Tesarik <ptesarik(a)suse.cz>
Everything *looks* correct -- but this patch doesn't fix anything, while
having the capacity to break what works. The slab-related changes still
concern me, at least until I test them.
As it turns out, I can't even test the slab changes, because I don't have
any more sample dumpfiles that go that far back to "pre-percpu" days. That
being the case, I'm not going to change the parts that modify dump_slab(),
dump_slab_objects() and gather_slab_free_list(). Why bother? It's
essentially dead code, but if there are still any users of it out there,
I'm not going to risk breaking it on them.
So I'm afraid it's going to continue to offend your sensibilities -- sorry
about that...
On the other hand, the changes to vm_area_dump() and next_upage() certainly
look harmless enough.
Thanks,
Dave
BTW, in the future, when you post patches can you add them as
attachments
instead of inlining them?
Thanks,
Dave
5:56 a.m.
Dne pátek 28 Leden 2011 22:10:23 Dave Anderson napsal(a):
----- Original Message -----
> ----- Original Message -----
>
> > I've been wondering about the intended use of this facilitator macro
> > for some time and concluded that it is plainly wrong. It should
> > take a pointer value from a buffer, but what is the use of a pointer
> > that pointed to something on the target machine?
> >
> > Since it cannot be meaningfully dereferenced on the host, the only
> > (arguable) advantage is the ability to do pointer arithmetics on the
> > variables. This second patch does the correct thing wherever pointer
> > arithmetics was done, making the actual calculations more explicit.
> >
> > Signed-off-by: Petr Tesarik <ptesarik(a)suse.cz>
>
> Everything *looks* correct -- but this patch doesn't fix anything, while
> having the capacity to break what works. The slab-related changes still
> concern me, at least until I test them.
As it turns out, I can't even test the slab changes, because I don't have
any more sample dumpfiles that go that far back to "pre-percpu" days. That
being the case, I'm not going to change the parts that modify dump_slab(),
dump_slab_objects() and gather_slab_free_list(). Why bother? It's
essentially dead code, but if there are still any users of it out there,
I'm not going to risk breaking it on them.
So I'm afraid it's going to continue to offend your sensibilities -- sorry
about that...
Hi Dave,
no need to be sorry. It's the same as Rik van Riel's Turtle and the Hare (see
http://surriel.com/system/files/summit2009-riel-turtle-and-hare.pdf). It's a
pleasure to work with you, because you've always been consistent in your
position. You're the turtle, I'm the hare. Occasionally, we can share some
bits, but I don't expect you to take my patches.
In fact, I'm more than happy with your conservative approach, because I'm
working for L3 Support here at SUSE, and we use the crash utility on a daily
basis. The low rate of regressions makes it possible to use the latest and
greatest version in production. That's unlike many other packages...
On the other hand, the distinction between a bugfix and a feature is a matter
of judgement, sometimes. You describe support for newer kernels as a bugfix
("fix for ... not working in Linux 2.6.x and later kernels"), but such things
would qualify as a feature request rather than a bug report at SUSE. The crash
utility had never worked with that new kernel.
And, in fact, my motivation to make a cross-platform crash is a perceived
regression by our partners. They're not so much interested in packages (and
code bases) as in the _functionality_ provided by those packages. So, they're
comparing crash with lcrash, which is cross-platform.
Regards,
Petr Tesarik
9:51 a.m.
----- Original Message -----
I've been wondering about the intended use of this facilitator
macro
for some time and concluded that it is plainly wrong. It should
take a pointer value from a buffer, but what is the use of a pointer
that pointed to something on the target machine?
Since it cannot be meaningfully dereferenced on the host, the only
(arguable) advantage is the ability to do pointer arithmetics on the
variables. This second patch does the correct thing wherever pointer
arithmetics was done, making the actual calculations more explicit.
Signed-off-by: Petr Tesarik <ptesarik(a)suse.cz>
The changes to vm_area_dump(), vm_area_page_dump() and next_upage()
have been queued for the next release.
Thanks,
Dave
5046
days inactive
5051
days old
devel@lists.crash-utility.osci.io
4 comments
2 participants
participants (2)
-
Dave Anderson -
Petr Tesarik