Improve 'inconsistent memattr' logging.
ClosedPublic
Actions

Authored by kib on Apr 29 2016, 11:07 AM.

Details

Reviewers

alc
jhb

Commits

rS298891: Avoid duplicated calls to pmap_page_get_memattr().

Summary

Avoid duplicated calls to pmap_page_get_memattr().

Avoid logging inconsistency for the /dev/mem device at all. The driver conscientiously ignores memattr, and the corrective action in the device pager handles it correctly.

In the message, name the driver we blame, and show memory attributes values.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

kib updated this revision to Diff 15731.Apr 29 2016, 11:07 AM

kib retitled this revision from to Improve 'inconsistent memattr' logging..

kib updated this object.

kib edited the test plan for this revision. (Show Details)

kib added reviewers: jhb, alc.

kib set the repository for this revision to rS FreeBSD src repository - subversion.

kib added a subscriber: pho.

Herald added a subscriber: imp. · View Herald TranscriptApr 29 2016, 11:07 AM

Can you elaborate on what you mean by "driver conscientiously ignores memattr"? At least, in cases where we read or write through the direct map, the direct map should already have the correct memory attribute set.

sys/vm/device_pager.c
331	Could you add a comment to explain why /dev/mem is a special case?

In D6149#130933, @alc wrote:

Can you elaborate on what you mean by "driver conscientiously ignores memattr"? At least, in cases where we read or write through the direct map, the direct map should already have the correct memory attribute set.

I mean, the mem driver keeps the value of the memattr passed to the driver d_mmap method, intact. The driver does not query the pmap for the current mapping mode of the page, to return in the *memattr. This is what causes the warnings printed.

OTOH, doing it in the mem driver would just duplicate the code which already exists in the device_pager.c, so the driver (correctly) does nothing in this regard, which I meant by '... conscientiously ignores ...'.

I agree that DMAP already has the right mapping mode, just that the warning is useless.

Tested on i386 and amd64 for a day. The WARNING is gone and I have not observed any issues with this patch.

Add comment about D_MEM exclusion.

There are at least two other drivers that set D_MEM: dev/firewire and i386/bios/smapi.c. However, dev/firewire doesn't support mmap. Are you sure that testing D_MEM is always going to be correct? Should this code really be checking the d_name field for "mem"?

sys/vm/device_pager.c
332	"For the /dev/mem ...
333	"memattr, pmap_page_get_memattr() ...

In D6149#131214, @alc wrote:

There are at least two other drivers that set D_MEM: dev/firewire and i386/bios/smapi.c. However, dev/firewire doesn't support mmap. Are you sure that testing D_MEM is always going to be correct? Should this code really be checking the d_name field for "mem"?

I thought that D_MEM specifically denoted /dev/mem (and /dev/kmem). The fact is that D_MEM is not used for anything right now (my added test is the first real use of it), and my impulse is to remove D_MEM from other drivers.

Alternatively, I can add another flag, but I do not see a reason.

Remove it from the other drivers. Can you find a way to assert that no other driver uses it? Otherwise, sooner or later, someone will cut-and-paste it from /dev/mem again.

Remove D_MEM from firewire and smapi. Improve comments.

In D6149#131229, @alc wrote:

Can you find a way to assert that no other driver uses it? Otherwise, sooner or later, someone will cut-and-paste it from /dev/mem again.

I do not see how to assert this without uglyness. Also, I think that other drivers might want to request the behaviour of /dev/mem there.

Instead, I put a comment near the D_MEM definition, hopefully this would make the driver author to pause for a second.

alc accepted this revision.May 1 2016, 5:26 PM

alc edited edge metadata.

This revision is now accepted and ready to land.May 1 2016, 5:26 PM

Closed by commit rS298891: Avoid duplicated calls to pmap_page_get_memattr(). (authored by kib). · Explain WhyMay 1 2016, 5:49 PM

This revision was automatically updated to reflect the committed changes.