On Wed, Jul 5, 2023 at 9:38 AM Peter Xu peterx@redhat.com wrote:
On Wed, Jul 05, 2023 at 09:27:15AM -0700, James Houghton wrote:
On Wed, Jul 5, 2023 at 9:15 AM Peter Xu peterx@redhat.com wrote:
On Wed, Jul 05, 2023 at 09:09:19AM -0700, James Houghton wrote:
diff --git a/include/linux/swapops.h b/include/linux/swapops.h index 4c932cb45e0b..8259fee32421 100644 --- a/include/linux/swapops.h +++ b/include/linux/swapops.h @@ -394,7 +394,8 @@ typedef unsigned long pte_marker;
#define PTE_MARKER_UFFD_WP BIT(0) #define PTE_MARKER_SWAPIN_ERROR BIT(1) -#define PTE_MARKER_MASK (BIT(2) - 1) +#define PTE_MARKER_UFFD_POISON BIT(2)
One more tab.
Though I remembered the last time we discussed IIRC we plan to rename SWAPIN_ERROR and reuse it, could you explain why a new bit is still needed?
I think I commented this but I'll do it again: IIUC any existing host swapin errors for guest pages should be reported as MCE too, afaict, happened in kvm context.
I think swapin errors are treated differently than poison. Swapin errors get VM_FAULT_SIGBUS, and poison gets VM_FAULT_HWPOISON, so UFFDIO_POISON should also get VM_FAULT_HWPOISON (so that's what Axel has implemented). And I think that needs a separate PTE marker.
My question was, should we also make SWAPIN_ERROR return VM_FAULT_HWPOISON always?
Just to recap from what I already commented above - if a guest page got error in swapin due to block sector failures, it should be treated as VM_FAULT_HWPOISON too, IMHO. IOW, I think current SWAPIN_ERROR is wrong when in kvm context and we should fix it first.
Oh! Yes, I agree, though I'm not familiar enough with the users of SWAPIN_ERROR to know if we can actually make this change.
Sorry I missed this, I'll take another pass looking at existing SWAPIN_ERROR uses, and see if this can be done.
Thanks for the thorough review Peter, I'll address this comment and the others in a v3. :)
Miaohe initially proposed this swapin error facility, let's see whether he can comment; he's already in the cc list.
AFAICT that's the right thing to do, and it shouldn't affect any existing user of swapin error if there is.
Or say, VM_FAULT_HWPOISON should be the same as VM_FAULT_SIGBUS when not in kvm context, so shouldn't change a thing in !kvm, while changing that should fix kvm from crashing a guest where we shouldn't need to.
-- Peter Xu