On 16.07.2018 15:04, Janosch Frank wrote:
> On 16.07.2018 14:02, David Hildenbrand wrote:
>> On 13.07.2018 15:20, Janosch Frank wrote:
>>> To do dirty loging with huge pages, we protect huge pmds in the
>>> gmap. When they are written to, we unprotect them and mark them dirty.
>>>
>>> We introduce the function gmap_test_and_clear_dirty_segment which
>>> handles dirty sync for huge pages.
>>>
>>> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
>>> ---
>>>
>>> So, that's a shot from the hip, I'll have to review this on Monday,
>>> but here's what David wanted.
>>
>> Looks cleaner to me!
>
>>> BUG_ON(gmap_is_shadow(gmap));
>>> @@ -598,12 +603,19 @@ int __gmap_link(struct gmap *gmap, unsigned long gaddr, unsigned long vmaddr)
>>> vmaddr >> PMD_SHIFT, table);
>>> if (!rc) {
>>> if (pmd_large(*pmd)) {
>>> - *table = pmd_val(*pmd) &
>>> - _SEGMENT_ENTRY_HARDWARE_BITS_LARGE;
>>> + *table = (pmd_val(*pmd) &
>>> + _SEGMENT_ENTRY_HARDWARE_BITS_LARGE)
>>> + | _SEGMENT_ENTRY_GMAP_UC;
>>> } else
>>> *table = pmd_val(*pmd) &
>>> _SEGMENT_ENTRY_HARDWARE_BITS;
>>> }
>>> + } else if (*table & _SEGMENT_ENTRY_PROTECT &&
>>> + !(pmd_val(*pmd) & _SEGMENT_ENTRY_PROTECT)) {
>>> + unprot = *table & _SEGMENT_ENTRY_HARDWARE_BITS_LARGE;
>>
>> As I said, this looks somewhat dangerous. It is okay to clear notifiers
>> (has to be done) but if we ever add another bit, this will silently get
>> erased here. Not sure how this could be done better. My take would be to
>> always let gmap_pmdp_xchg() clear the notifier bits in the new value and
>> not remove any bit except protection at this point.
>
> Alright, this will also solve, that we do not remove notifiers on
> gmap_protect_pmd right now...
>
> For split and VSIE I will add definitions for status and notifier bits,
> for easier masking, something like:
> #define _SEGMENT_ENTRY_NOTIFY_BITS (_SEGMENT_ENTRY_GMAP_IN |
> _SEGMENT_ENTRY_GMAP_VSIE)
>
> #define _SEGMENT_ENTRY_STATUS_BITS (_SEGMENT_ENTRY_GMAP_UC |
> _SEGMENT_ENTRY_SPLIT)
Makes sense, I wonder if we should put GMAP somewhere in there ...
>
>
> I applied all of your other changes, except for the find_first_bit
> stuff, I'll worry about that later.
>
--
Thanks,
David / dhildenb
