Re: svn commit: r308817 - head/sys/powerpc/include [Still have pmap_t and struct pmap ppowerpc64 problems as of -r308860]

From: Justin Hibbits <jhibbits_at_freebsd.org>
Date: Sun, 20 Nov 2016 00:12:30 -0600
My buildworld completed successfully, so it's been fixed in r308873/ 
r308874.

Thanks for your testing.  I often build just kernel, so wouldn't have  
seen the fallout until it was far too late.

- Justin

On Nov 19, 2016, at 10:22 PM, Mark Millard wrote:

> On 2016-Nov-16, at 8:33 PM, Justin Hibbits <jhibbits_at_freebsd.org>  
> wrote:
>
>> *sigh* okay, thanks.  I just tested, and vm/vm_page.h, and vm/vm.h  
>> can both be removed from memstat_uma.c for it to compile.  I'm  
>> kicking off a buildworld myself now, too, and hope to have it ready  
>> to commit tomorrow (takes a couple hours to buildworld on my G5).
>>
>> - Justin
>
> That will not be the only potential place: umastat.c in tools/tools/ 
> umastat/
> also has a include of vm/vm_page.h:
>
>> # find /usr/src/ -name .svn -prune -o -name sys -prune -o -name man  
>> -prune -o -exec grep "vm_page[.]h" {} \; -print | more
>> #include <vm/vm_page.h>
>> /usr/src/lib/libmemstat/memstat_uma.c
>> #define LIBMEMSTAT      /* Cause vm_page.h not to include  
>> opt_vmpage.h */
>> #include <vm/vm_page.h>
>> /usr/src/tools/tools/umastat/umastat.c
>
>
> ===
> Mark Millard
> markmi at dsl-only.net
>
> On Nov 19, 2016, at 9:47 PM, Mark Millard wrote:
>
>> [Top post of bad news.]
>>
>> With the patch I get a different incomplete type used in libmemstat:
>>
>> struct md_page
>>
>> --- all_subdir_lib/libmemstat ---
>> In file included from /usr/src/lib/libmemstat/memstat_uma.c:34:0:
>> /usr/src/sys/vm/vm_page.h:144:17: error: field 'md' has incomplete  
>> type
>> struct md_page md;  /* machine dependent stuff */
>>               ^
>> *** [memstat_uma.o] Error code 1
>>
>> make[5]: stopped in /usr/src/lib/libmemstat
>>
>>
>> ===
>> Mark Millard
>> markmi at dsl-only.net
>>
>> On 2016-Nov-19, at 7:42 PM, Mark Millard <markmi at dsl-only.net>  
>> wrote:
>>
>>> On 2016-Nov-19, at 7:36 PM, Mark Millard <markmi at dsl-only.net>  
>>> wrote:
>>>
>>>> On 2016-Nov-19, at 7:32 PM, Justin Hibbits <jhibbits at  
>>>> freebsd.org> wrote:
>>>>
>>>>> Sorry, I generated the diff from a different tree that wasn't  
>>>>> synced to head (had the same change in both trees originally).  
>>>>> If that is the only problem, you can ignore it and try the rest.  
>>>>> I can generate another diff later too.
>>>>> - Justin
>>>>
>>>> Yep: I manually did the move of the pm_stats line and am building.
>>>
>>> If it builds and I install it on a PowerMac G5 and it boots, what  
>>> do I
>>> do to test if pm_stats and pm_mtx seems to be working well/right for
>>> the out of kernel code? Do you know of a reasonable test?
>>>
>>> ===
>>> Mark Millard
>>> markmi at  dsl-only.net
>>>
>> On Nov 19, 2016 21:27, "Mark Millard" <markmi at dsl-only.net> wrote:
>>> [Top post about patch issues.]
>>>
>>> Looking at the patch it seems to be designed for when #else was in  
>>> use:
>>>
>>>> -#else
>>>> +#elif defined(BOOKE)
>>>
>>> but -r308817 already has the 2nd line (BOOKE). Your patch shows:
>>>
>>>> Index: sys/powerpc/include/pmap.h
>>>> ===================================================================
>>>> --- sys/powerpc/include/pmap.h  (revision 308718)
>>>> +++ sys/powerpc/include/pmap.h  (working copy)
>>>
>>> So it looks like you started from before -r308817 .
>>>
>>> Trying it (I'm at -r308860):
>>>
>>>> Patching file sys/powerpc/include/pmap.h using Plan A...
>>>> Hunk #1 succeeded at 74.
>>>> Hunk #2 succeeded at 84.
>>>> Hunk #3 succeeded at 132.
>>>> Hunk #4 succeeded at 145.
>>>> Hunk #5 failed at 180.
>>>> Hunk #6 succeeded at 194.
>>>> Hunk #7 succeeded at 210.
>>>> 1 out of 7 hunks failed--saving rejects to sys/powerpc/include/ 
>>>> pmap.h.rej
>>>
>>>> # more sys/powerpc/include/pmap.h.rej
>>>> _at__at_ -179,13 +180,13 _at__at_
>>>> struct slb **slb_alloc_user_cache(void);
>>>> void   slb_free_user_cache(struct slb **);
>>>>
>>>> -#else
>>>> +#elif defined(BOOKE)
>>>>
>>>> struct pmap {
>>>> +       struct pmap_statistics  pm_stats;       /* pmap  
>>>> statistics */
>>>>    struct mtx              pm_mtx;         /* pmap mutex */
>>>>    tlbtid_t                pm_tid[MAXCPU]; /* TID to identify  
>>>> this pmap entries in TLB */
>>>>    cpuset_t                pm_active;      /* active on cpus */
>>>> -       struct pmap_statistics  pm_stats;       /* pmap  
>>>> statistics */
>>>>
>>>>    /* Page table directory, array of pointers to page tables. */
>>>>    pte_t                   *pm_pdir[PDIR_NENTRIES];
>>>
>>>
>>> ===
>>> Mark Millard
>>> markmi at dsl-only.net
>>>
>>> On 2016-Nov-19, at 7:00 PM, Mark Millard <markmi at dsl-only.net>  
>>> wrote:
>>>
>>> It may take a little bit but I'll try the patch.
>>>
>>> It looks like sys/powerpc/include/pmap.h from -r176700 from 2088- 
>>> Mar-3
>>> is when the BOOKE/E500 split started with the preprocessor use of  
>>> AIM
>>> and #else . This predates PowerMac G5 support.
>>>
>>> This is definitely not new for the general structure on the powerpc
>>> side of things. Any place that did not have the AIM vs. not status
>>> available was subject to problems of possibly mismatched  
>>> definitions.
>>>
>>> ===
>>> Mark Millard
>>> markmi at dsl-only.net
>>>
>>> On 2016-Nov-19, at 6:47 PM, Justin Hibbits <jhibbits at  
>>> freebsd.org> wrote:
>>>
>>> On Sat, 19 Nov 2016 18:36:39 -0800
>>> Mark Millard <markmi at dsl-only.net> wrote:
>>>
>>>> [Quick top post I'm afraid.]
>>>>
>>>> I think that I figured out why there is a problem even earlier
>>>> --that just did not stop the compiles.
>>>>
>>>> lib/libutil/kinfo_getallproc.c is built here as part of buildworld
>>>> (stage 4.2 "building libraries" instead of buildkernel. It does not
>>>> have the KERNCONF's AIM vs. BOOKE vs. . . . definitions vs. lack of
>>>> them).
>>>>
>>>> So if it includes machine/pmap.h that binds to
>>>> sys/powerpc/include/pmap.h which has the structure. . .
>>>>
>>>> . . .
>>>> #if defined(AIM)
>>>> . . . (definitions here)
>>>> #elif defined(BOOKE)
>>>> . . . (definitions here)
>>>> #endif
>>>> . . .
>>>>
>>>> it gets no definition now.
>>>>
>>>> With the older:
>>>>
>>>> . . .
>>>> #if defined(AIM)
>>>> . . . (definitions here)
>>>> #else
>>>> . . . (definitions here)
>>>> #endif
>>>> . . .
>>>>
>>>> It got a definition, just not necessarily the right one.
>>>>
>>>>
>>>> ===
>>>> Mark Millard
>>>> markmi at dsl-only.net
>>>
>>> Can you try the attached patch?  There was a subtle ABI issue that
>>> r308817 exposed, which is that the pmap structs aren't identical  
>>> such
>>> that the pm_stats are at different locations, and libkvm ends up
>>> reading with the Book-E pmap, getting different stats than  
>>> expected for
>>> AIM.  This patch fixes that, bumping version to account for this ABI
>>> change.
>>>
>>> - Justin<fix_pmap.diff>
>>
>>
>>
>
>
Received on Sun Nov 20 2016 - 05:12:33 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:41:08 UTC