Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1175555 > unrolled thread

Re: [PATCH 05/11] mm: debug: dump page into a string rather than directly on screen

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 05/11] mm: debug: dump page into a string rather than  directly on screen David Rientjes <rientjes@google.com> - 2015-07-01 23:30 +0200
    Re: [PATCH 05/11] mm: debug: dump page into a string rather than  directly on screen "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-07-01 23:40 +0200
    Re: [PATCH 05/11] mm: debug: dump page into a string rather than  directly on screen Vlastimil Babka <vbabka@suse.cz> - 2015-07-02 00:40 +0200
    Re: [PATCH 05/11] mm: debug: dump page into a string rather than  directly on screen Sasha Levin <sasha.levin@oracle.com> - 2015-07-02 01:00 +0200

#1175555 — Re: [PATCH 05/11] mm: debug: dump page into a string rather than directly on screen

On Wed, 1 Jul 2015, Sasha Levin wrote:

> On 06/30/2015 07:35 PM, David Rientjes wrote:
> > I don't know how others feel, but this looks strange to me and seems like 
> > it's only a result of how we must now dump page information 
> > (dump_page(page) is no longer available, we must do pr_alert("%pZp", 
> > page)).
> > 
> > Since we're relying on print formats, this would arguably be better as
> > 
> > 	pr_alert("Not movable balloon page:\n");
> > 	pr_alert("%pZp", page);
> > 
> > to avoid introducing newlines into potentially lengthy messages that need 
> > a specified loglevel like you've done above.
> > 
> > But that's not much different than the existing dump_page() 
> > implementation.
> > 
> > So for this to be worth it, it seems like we'd need a compelling usecase 
> > for something like pr_alert("%pZp %pZv", page, vma) and I'm not sure we're 
> > ever actually going to see that.  I would argue that
> > 
> > 	dump_page(page);
> > 	dump_vma(vma);
> > 
> > would be simpler in such circumstances.
> 
> I think we can find usecases where we want to dump more information than what's
> contained in just one page/vma/mm struct. Things like the following from mm/gup.c:
> 
> 	VM_BUG_ON_PAGE(compound_head(page) != head, page);
> 
> Where seeing 'head' would be interesting as well.
> 

I think it's a debate about whether this would be better off handled as

	if (VM_BUG_ON(compound_head(page) != head)) {
		dump_page(page);
		dump_page(head);
	}

and avoid VM_BUG_ON_PAGE() and the new print formats entirely.  We can 
improve upon existing VM_BUG_ON(), and BUG_ON() itself since the VM isn't 
anything special in this regard, to print diagnostic information that may 
be helpful, but I don't feel like adding special VM_BUG_ON_*() macros or 
printing formats makes any of this simpler.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1175562

On Wed, Jul 01, 2015 at 02:25:56PM -0700, David Rientjes wrote:
> On Wed, 1 Jul 2015, Sasha Levin wrote:
> 
> > On 06/30/2015 07:35 PM, David Rientjes wrote:
> > > I don't know how others feel, but this looks strange to me and seems like 
> > > it's only a result of how we must now dump page information 
> > > (dump_page(page) is no longer available, we must do pr_alert("%pZp", 
> > > page)).
> > > 
> > > Since we're relying on print formats, this would arguably be better as
> > > 
> > > 	pr_alert("Not movable balloon page:\n");
> > > 	pr_alert("%pZp", page);
> > > 
> > > to avoid introducing newlines into potentially lengthy messages that need 
> > > a specified loglevel like you've done above.
> > > 
> > > But that's not much different than the existing dump_page() 
> > > implementation.
> > > 
> > > So for this to be worth it, it seems like we'd need a compelling usecase 
> > > for something like pr_alert("%pZp %pZv", page, vma) and I'm not sure we're 
> > > ever actually going to see that.  I would argue that
> > > 
> > > 	dump_page(page);
> > > 	dump_vma(vma);
> > > 
> > > would be simpler in such circumstances.
> > 
> > I think we can find usecases where we want to dump more information than what's
> > contained in just one page/vma/mm struct. Things like the following from mm/gup.c:
> > 
> > 	VM_BUG_ON_PAGE(compound_head(page) != head, page);
> > 
> > Where seeing 'head' would be interesting as well.
> > 
> 
> I think it's a debate about whether this would be better off handled as
> 
> 	if (VM_BUG_ON(compound_head(page) != head)) {
> 		dump_page(page);
> 		dump_page(head);

Huh? How would we reach this, if VM_BUG_ON() will trigger BUG()?

> 	}
> 
> and avoid VM_BUG_ON_PAGE() and the new print formats entirely.  We can 
> improve upon existing VM_BUG_ON(), and BUG_ON() itself since the VM isn't 
> anything special in this regard, to print diagnostic information that may 
> be helpful, but I don't feel like adding special VM_BUG_ON_*() macros or 
> printing formats makes any of this simpler.

-- 
 Kirill A. Shutemov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1175584

On 1.7.2015 23:25, David Rientjes wrote:
> On Wed, 1 Jul 2015, Sasha Levin wrote:
> 
>> On 06/30/2015 07:35 PM, David Rientjes wrote:
>>
>> I think we can find usecases where we want to dump more information than what's
>> contained in just one page/vma/mm struct. Things like the following from mm/gup.c:
>>
>> 	VM_BUG_ON_PAGE(compound_head(page) != head, page);
>>
>> Where seeing 'head' would be interesting as well.
>>
> 
> I think it's a debate about whether this would be better off handled as
> 
> 	if (VM_BUG_ON(compound_head(page) != head)) {
> 		dump_page(page);
> 		dump_page(head);
> 	}
> 
> and avoid VM_BUG_ON_PAGE() and the new print formats entirely.  We can 
> improve upon existing VM_BUG_ON(), and BUG_ON() itself since the VM isn't 
> anything special in this regard,

Well, BUG_ON() is just evaluating a condition that results in executing the UD2
instruction, which traps and the handler prints everything. The file:line info
it prints is emitted in a different section, and the handler has to search for
it to print it, using the trapping address. This all to minimize impact on I$,
branch predictors and whatnot.

VM_BUG_ON_PAGE() etc have to actually emit the extra printing code before
triggering UD2. I'm not sure if there's a way to extend the generic mechanism
here. The file:line info would have to also include information about the extra
things we want to dump, and where the handler would find the necessary pointers
(in the registers saved on UD2 exception, or stack). This could probably be done
with some dwarf debuginfo magic but we know how unreliable that can be. Some of
the data might already be discarded in the non-error path doesn't need it, so it
would have to make sure to store it somewhere for the error purposes.

Now we seem to accept that VM_BUG_ON* is more intrusive than BUG_ON() and it's
not expected to be enabled in default distro kernels etc., so it can afford to
pollute the code with extra prints...

> to print diagnostic information that may 
> be helpful, but I don't feel like adding special VM_BUG_ON_*() macros or 
> printing formats makes any of this simpler.
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1175593

On 07/01/2015 05:25 PM, David Rientjes wrote:
> On Wed, 1 Jul 2015, Sasha Levin wrote:
> 
>> On 06/30/2015 07:35 PM, David Rientjes wrote:
>>> I don't know how others feel, but this looks strange to me and seems like 
>>> it's only a result of how we must now dump page information 
>>> (dump_page(page) is no longer available, we must do pr_alert("%pZp", 
>>> page)).
>>>
>>> Since we're relying on print formats, this would arguably be better as
>>>
>>> 	pr_alert("Not movable balloon page:\n");
>>> 	pr_alert("%pZp", page);
>>>
>>> to avoid introducing newlines into potentially lengthy messages that need 
>>> a specified loglevel like you've done above.
>>>
>>> But that's not much different than the existing dump_page() 
>>> implementation.
>>>
>>> So for this to be worth it, it seems like we'd need a compelling usecase 
>>> for something like pr_alert("%pZp %pZv", page, vma) and I'm not sure we're 
>>> ever actually going to see that.  I would argue that
>>>
>>> 	dump_page(page);
>>> 	dump_vma(vma);
>>>
>>> would be simpler in such circumstances.
>>
>> I think we can find usecases where we want to dump more information than what's
>> contained in just one page/vma/mm struct. Things like the following from mm/gup.c:
>>
>> 	VM_BUG_ON_PAGE(compound_head(page) != head, page);
>>
>> Where seeing 'head' would be interesting as well.
>>
> 
> I think it's a debate about whether this would be better off handled as
> 
> 	if (VM_BUG_ON(compound_head(page) != head)) {
> 		dump_page(page);
> 		dump_page(head);
> 	}

Since we'd BUG at VM_BUG_ON(), this would be something closer to:

	if (unlikely(compound_head(page) != head)) {
		dump_page(page);
		dump_page(head);
		VM_BUG_ON(1);
	}

But my point here was that while one *could* do it that way, no one does because
it's not intuitive. We both agree that in the example above it would be useful to
see both 'page' and 'head', and yet the code that was written didn't dump any of
them. Why? No one wants to write debug code unless it's easy and short.

> and avoid VM_BUG_ON_PAGE() and the new print formats entirely.  We can 
> improve upon existing VM_BUG_ON(), and BUG_ON() itself since the VM isn't 
> anything special in this regard, to print diagnostic information that may 
> be helpful, but I don't feel like adding special VM_BUG_ON_*() macros or 
> printing formats makes any of this simpler.

This patchset actually kills the VM_BUG_ON_*() macros for exactly that reason:
VM isn't special at all and doesn't need it's own magic code in the form of
VM_BUG_ON_*() macros and dump_*() functions.


Thanks,
Sasha

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web