svn commit: r327950 - in head/sys/powerpc: aim include powerpc ps3

[Nathan Whitehorn]

On 01/15/18 09:06, Konstantin Belousov wrote:
> On Mon, Jan 15, 2018 at 07:33:01AM -0800, Nathan Whitehorn wrote:
>>
>> On 01/15/18 03:18, Konstantin Belousov wrote:
>>> On Sun, Jan 14, 2018 at 03:46:38PM -0800, Nathan Whitehorn wrote:
>>>> On 01/14/18 15:42, Nathan Whitehorn wrote:
>>>>> On 01/14/18 09:57, Nathan Whitehorn wrote:
>>>>>> On 01/14/18 09:52, Konstantin Belousov wrote:
>>>>>>> On Sun, Jan 14, 2018 at 09:30:53AM -0800, Nathan Whitehorn wrote:
>>>>>>>> The immediate consequence of that is that no MI code that knows about
>>>>>>>> direct maps can possibly take advantage of the direct map on this
>>>>>>>> platform. Do we really want that to save some conditional logic that
>>>>>>>> would get optimized out on amd64 and arm64 anyway? I really do not see
>>>>>>>> the benefit here.
>>>>>>> It is not clear what do you mean.  Are you saying that there is no
>>>>>>> benefit
>>>>>>> of providing the conditional logic, or that it is not benefit of
>>>>>>> exclusing
>>>>>>> powerpc ?
>>>>>> Sorry, that was poorly stated. Let me try again:
>>>>>>
>>>>>> If we make a PPC_PHYS_TO_DMAP(), but there is an MI PHYS_TO_DMAP()
>>>>>> API, consumer code in the MI parts of the kernel won't be able to
>>>>>> benefit from the PPC direct map, which seems unfortunate. The cost
>>>>>> from a code perspective of having an if (direct_map_available) seems
>>>>>> low, since on systems where direct_map_available is defined to be 1,
>>>>>> the compiler will optimize it to the same code as if gated by #ifdef.
>>>>>> It might be more cumbersome to write the code, however.
>>>>>>
>>>>>>> I do not object against adding the conditional, but it should not be
>>>>>>> too clumsy to use.
>>>>>>>
>>>>>> OK. Let me try to draft something in the next couple days and see how
>>>>>> much of a pain it is in practice.
>>>>>> -Nathan
>>>>>>
>>>>> How about the attached? It makes PHYS_TO_DMAP() return 0 if no mapping
>>>>> exists. This is straightforward, does not introduce extra macros, and
>>>>> can pretty easily replace SFBUF_OPTIONAL_DIRECT_MAP on the assumption
>>>>> that PHYS_TO_DMAP() is cheap. I've modified the other MI-ish consumers
>>>>> in the tree accordingly; compat/linuxkpi/common/src/linux_page.c
>>>>> already does the right thing and needed no modifications.
>>>>> -Nathan
>>> I think that this is fine from the PoV of code complexity.
>>>
>>> We now require MI (but not amd64 and arm64 MD) code to check for
>>> PHYS_TO_DMAP() return value, which is redundand for a*64. I am not sure
>>> if this is good choice from the PoV of possible microoptimizations.
>>> You promised something which is trivially detectable by compiler as
>>> an excess code.
>> Fair enough -- the logic was that a lot of code already checks for NULL
>> pointers (the linux_page.c for instance required no changes to do the
>> right thing).
> Most likely this is an accidental feature of the linux code and not the
> specific decision by the freebsd emulation of it.
>
>> If we want it to be fully compiler-transparent, we could
>> also add a flag, but that would add more code complexity. Do you have a
>> preference? I would be happy to draft that too.
> I think I am fine with amd64 doing
> #define	PMAP_HAS_DMAP	1
> in machine/param.h.  I do not insist on the name.
>
> Then ppc could define its version as a reference to the variable.  I thought
> that might be you can create less clumsy model of propagating this to the
> MI VM level.

That seems fine to me. I don't think a less-clumsy way that does not 
involve extra indirection is possible. The PHYS_TO_DMAP() returning NULL 
is about the best thing I can come up with from a clumsiness standpoint 
since plenty of code checks for null pointers already, but doesn't 
cleanly handle the rarer case where you want to test for the existence 
of direct maps in general without testing some potemkin address.

My one reservation about PMAP_HAS_DMAP or the like as a selector is that 
it does not encode the full shape of the problem: one could imagine 
having a direct map that only covers a limited range of RAM (I am not 
sure whether the existence of dmaplimit on amd64 implies this can happen 
with non-device memory in real life), for example. These cases are 
currently covered by an assert() in PHYS_TO_DMAP(), whereas having 
PHYS_TO_DMAP() return NULL allows a more flexible signalling and the 
potential for the calling code to do something reasonable to handle the 
error. A single global flag can't convey information at this kind of 
granularity. Is this a reasonable concern? Or am I overthinking things?
-Nathan