On 5/9/2022 1:46 PM, Christophe Leroy wrote:
Sorry for my misunderstanding. I just follow the explicit void casting like other places in hugetlb.c file. And I am not sure if it is useful adding some comments like below, since we did not need the original pte value in the COW case mapping with a new page, and the code is more readable already I think.Le 08/05/2022 à 15:09, Baolin Wang a écrit :On 5/8/2022 7:09 PM, Muchun Song wrote:On Sun, May 08, 2022 at 05:36:39PM +0800, Baolin Wang wrote:It is incorrect to use ptep_clear_flush() to nuke a hugetlb page table when unmapping or migrating a hugetlb page, and will change to use huge_ptep_clear_flush() instead in the following patches. So this is a preparation patch, which changes the huge_ptep_clear_flush() to return the original pte to help to nuke a hugetlb page table. Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx> Acked-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx>Reviewed-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>Thanks for reviewing.But one nit below: [...]diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 8605d7e..61a21af 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -5342,7 +5342,7 @@ static vm_fault_t hugetlb_wp(struct mm_struct *mm, struct vm_area_struct *vma, ClearHPageRestoreReserve(new_page); /* Break COW or unshare */ - huge_ptep_clear_flush(vma, haddr, ptep); + (void)huge_ptep_clear_flush(vma, haddr, ptep);Why add a "(void)" here? Is there any warning if no "(void)"? IIUC, I think we can remove this, right?I did not meet any warning without the casting, but this is per Mike's comment[1] to make the code consistent with other functions casting to void type explicitly in hugetlb.c file. [1] https://lore.kernel.org/all/495c4ebe-a5b4-afb6-4cb0-956c1b18d0cc@xxxxxxxxxx/As far as I understand, Mike said that you should be accompagnied with a big fat comment explaining why we ignore the returned value from huge_ptep_clear_flush(). > By the way huge_ptep_clear_flush() is not declared 'must_check' so this cast is just visual polution and should be removed. In the meantime the comment suggested by Mike should be added instead.
Mike, could you help to clarify what useful comments would you like? and remove the explicit void casting? Thanks.
/* * Just ignore the return value with new page mapped. */
|