Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <487fdaad-0873-142a-cf3a-a065bc8acff6@nvidia.com>
Date:   Tue, 15 Feb 2022 13:50:44 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.0
Subject: Re: [PATCH v2 1/3] migrate.c: Remove vma check in migrate_vma_setup()
Content-Language: en-US
To:     Alistair Popple <apopple@nvidia.com>, akpm@linux-foundation.org,
        linux-mm@kvack.org
Cc:     Felix.Kuehling@amd.com, rcampbell@nvidia.com,
        linux-ext4@vger.kernel.org, linux-xfs@vger.kernel.org,
        amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
        hch@lst.de, jgg@nvidia.com, jglisse@redhat.com,
        willy@infradead.org, alex.sierra@amd.com
References: <cover.0d3c846b1c6c294e055ff7ebe221fab9964c1436.1644207242.git-series.apopple@nvidia.com>
 <6831bf69f7c7699be83b31c9c56212b5fb07f873.1644207242.git-series.apopple@nvidia.com>
From:   John Hubbard <jhubbard@nvidia.com>
In-Reply-To: <6831bf69f7c7699be83b31c9c56212b5fb07f873.1644207242.git-series.apopple@nvidia.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Y0o1ZVliR3MyMU9Hdk50clk2VGdIQzdBdDUzTHZsejlZeTUrWFhuWTBUdVhn?=
 =?utf-8?B?NEtienpwaE1tcGU4UUJHRGt0b1ZHUmtreW5IckFHVDdROGQ3aVFPVGJRODFR?=
 =?utf-8?B?SDc1ampDaFA4eUdMd1JtcVJCRjFRTm9FbVorZ3lYSHp1UXFvOWVPNjhKWlh4?=
 =?utf-8?B?RHR0Z2ZGR082QVhjaXBRaElMQldFU0t3NXB6WTRUdHlWZlJTdW5jbDBOeGJs?=
 =?utf-8?B?OHZCUGY0ZkRwcUdYQnB2NXE4NkExeWhPd2JYRjBtYUFnSFdpL1dEVXYxb1F2?=
 =?utf-8?B?QUtVUFYybFFqL3p3VklCcWFQUEp3VkNvMmpneExqYUFDUy9GWUg1QThweXdm?=
 =?utf-8?B?TDlEZHl6QjVsZEMzMXA0S0dSYkRkcXpleFovSzR2YUZyVWhRUUhBY2JZNnJ0?=
 =?utf-8?B?bVFGSGFmMTl2U1hsaTIvUC9jWUwxN0FxWnRMOER0RTFUTlpiSTVLbTdhVnpu?=
 =?utf-8?B?QmxuaWZ3MHhrMU90cU5KZk5YelVoNmFNOVJ6ZFpTcG8rSUMwNUp6YUQ4RnR6?=
 =?utf-8?B?RU1xeFVpNnlFQ0swd1ltdXpjNEUvbE4xT0trdHlrSmlWQUNaNTB4UXF2WG1K?=
 =?utf-8?B?MVg5KzV3VE1FU2FDRERISndDT0NKZUt3Vk1pQ0NlTWQ5Wm0xNmF0c0hBeTZ2?=
 =?utf-8?B?RVFFU2drNnRFTElhT0xCTllZdWNjOVgyNXZ3bWg5My9peWRjdWVKT3hXeGZ5?=
 =?utf-8?B?T3daRnorQytVTjJnTWx5a05vVk12K2k4RUFCc0NWRHFtdlJMeDNKbS96djBC?=
 =?utf-8?B?RVFMdFZnOTcwd1hYZkJjSnk0Wi9CT2xhUFFyTjFibStmMk9XYk85YzhtWEhH?=
 =?utf-8?B?akZ5dFdab1FiNTdxMlF6L1RRaHF2dUk2Z1N3NStmaEg1ZnRveTBLUkNYTUJy?=
 =?utf-8?B?SVlNdUlVZ2Y2WFRFTXFDRjljcWNTSmlSZzE5WFhyYlRubExyREcvdDJJQm9M?=
 =?utf-8?B?eGtMUGErMURTbE9aeXFqVG5PMThYR0RNMHJnMVcyalpKUEt2aGRzUkhaWEZI?=
 =?utf-8?B?RjhZaEN2bzJYaTNsWmN1TTlXYVFGMWdGZlFodTFrWlJsMU5zSmdUMmw0alA1?=
 =?utf-8?B?TzJ1NVVsNWtDSVN3WE9nTUE2dHRWWitmd2MyQnNWNUJjUFh2c2Q0dmhxT2ZI?=
 =?utf-8?B?WnhaVG50ckNpUUloSkd4NHRrUmpsckM2UzFuYkJBV3ZOQXlkaDREa3NjTjBU?=
 =?utf-8?B?cDZSb1RkMmtEdUhLRk51MmtDWDNrQXNzdS9oMmxOZWxPMDNvMUdWVkZqK2VM?=
 =?utf-8?B?bEUyK0ZDSHJ1U1ZpMDhESGdtNnAxQUVPMTNhMHZnUVQrV1Y1U283TDJhUUJC?=
 =?utf-8?B?NU1rUUZFWUx6djY3Yy9sMlZ6T29TZ3hGQ0x4S2xTajRabUd2RlhJQnB1bW9j?=
 =?utf-8?B?cDlhU0Zpa3k0bE9qK1RWL09MS1A2TjQwUHNOc3dkNFdlOFFDeVhmR1F6QjdI?=
 =?utf-8?B?STZvUWMySy9GUkdSN3VaN2NBa2QyaEVnLzhEVVFQQmQ4M0pFVXBFRlo0NS9j?=
 =?utf-8?B?dFdWK2xvMFZTaXdIeXFRSmc5UFRZNVVvSnNKVzBPdXoxNXV0dWVENEwvd1RI?=
 =?utf-8?B?UFJWV3ZCTjhiUFpRU1d2bzhsMTNic1k3M2FQVDkwakVub3czMXU1ZU44bG9B?=
 =?utf-8?B?Y0h2YW5ROW5SK1doRHlRdGtoUGkzL1V6RjNkNXA0VjZFTkhrTkc4eVNiaE1a?=
 =?utf-8?B?RlJOM3o5UFpIaDhrYndtbWwrSTM4MUtNRW5iOGh0ZkpvTE5TeUQwVjcvalBR?=
 =?utf-8?B?SzhCWlVyaWFseXJLcVhRK1BCamxtTUNNOEJFWkxHOEpTSkJ5ejFoODRLbENV?=
 =?utf-8?B?a0U4Tmw5TGIrTWowdVB3eTRjYUZqandNZGN6ZS9odXlCd2tKRkZUUmFKR1RI?=
 =?utf-8?B?V1UzWTMyUkZidjZiaFZHbG4rK1lkcmdTSWVPS20xZmYvZzkrR0hwZndsTUE0?=
 =?utf-8?B?RFAyVS9iMmFqUlVYRTZMTU5kRGUzODd5U0dzRWRFSTJzUFpvN3p6NVZyRmVy?=
 =?utf-8?B?aVBIMWMzRExCSnR1bnVnWk12STNkRTZMSXE5cmxYdCtLU3RTRnlEOUgxRFRa?=
 =?utf-8?B?SmVxeFRXTVBIaUVQOGhROFZnVDNFR1RLWXpucDBvTXZWb2YyMFdKczY3WGtn?=
 =?utf-8?B?UDQwY1NBY0NJNEpzY1BseGJZQ0M0Q1JSN2RTUzhZOTFad2lNY1FkTFR4bVg3?=
 =?utf-8?Q?2Af13epG7JE5NSkIAxLZL+M=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 7fab9d82-0ce1-45fd-433b-08d9f0cd3928
X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB4130.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Feb 2022 21:50:46.9256
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: pbcMlCqV1Pa0dmV/cTuSc9xwfQ7+6rXCvfAbTB/dfYYBZY4pCsRbPBNiZd85QeCJh6mpvOK2PI11z1FDxodXOw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR12MB3620
X-Spam-Status: No, score=-1.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-ext4.vger.kernel.org>
X-Mailing-List: linux-ext4@vger.kernel.org

On 2/6/22 20:26, Alistair Popple wrote:
> migrate_vma_setup() checks that a valid vma is passed so that the page
> tables can be walked to find the pfns associated with a given address
> range. However in some cases the pfns are already known, such as when
> migrating device coherent pages during pin_user_pages() meaning a valid
> vma isn't required.
> 
> Signed-off-by: Alistair Popple <apopple@nvidia.com>
> Acked-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
> 
> Changes for v2:
> 
>   - Added Felix's Acked-by
> 
>   mm/migrate.c | 34 +++++++++++++++++-----------------
>   1 file changed, 17 insertions(+), 17 deletions(-)
> 

Hi Alistair,

Another late-breaking review question, below. :)

> diff --git a/mm/migrate.c b/mm/migrate.c
> index a9aed12..0d6570d 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2602,24 +2602,24 @@ int migrate_vma_setup(struct migrate_vma *args)
>   
>   	args->start &= PAGE_MASK;
>   	args->end &= PAGE_MASK;
> -	if (!args->vma || is_vm_hugetlb_page(args->vma) ||
> -	    (args->vma->vm_flags & VM_SPECIAL) || vma_is_dax(args->vma))
> -		return -EINVAL;
> -	if (nr_pages <= 0)
> -		return -EINVAL;

Was the above check left out intentionally? If so, then it needs a
commit description note. And maybe even a separate patch, because it
changes the behavior.

If you do want to change the behavior:

* The kerneldoc comment above this function supports such a change: it
requires returning 0 for the case of zero pages requested. So your
change would bring the comments into alignment with the code.

* I don't think memset deals properly with a zero length input arg, so
it's probably better to return 0, before that point.


thanks,
-- 
John Hubbard
NVIDIA

> -	if (args->start < args->vma->vm_start ||
> -	    args->start >= args->vma->vm_end)
> -		return -EINVAL;
> -	if (args->end <= args->vma->vm_start || args->end > args->vma->vm_end)
> -		return -EINVAL;
>   	if (!args->src || !args->dst)
>   		return -EINVAL;
> -
> -	memset(args->src, 0, sizeof(*args->src) * nr_pages);
> -	args->cpages = 0;
> -	args->npages = 0;
> -
> -	migrate_vma_collect(args);
> +	if (args->vma) {
> +		if (is_vm_hugetlb_page(args->vma) ||
> +			(args->vma->vm_flags & VM_SPECIAL) || vma_is_dax(args->vma))
> +			return -EINVAL;
> +		if (args->start < args->vma->vm_start ||
> +			args->start >= args->vma->vm_end)
> +			return -EINVAL;
> +		if (args->end <= args->vma->vm_start || args->end > args->vma->vm_end)
> +			return -EINVAL;
> +
> +		memset(args->src, 0, sizeof(*args->src) * nr_pages);
> +		args->cpages = 0;
> +		args->npages = 0;
> +
> +		migrate_vma_collect(args);
> +	}
>   
>   	if (args->cpages)
>   		migrate_vma_unmap(args);
> @@ -2804,7 +2804,7 @@ void migrate_vma_pages(struct migrate_vma *migrate)
>   			continue;
>   		}
>   
> -		if (!page) {
> +		if (!page && migrate->vma) {
>   			if (!(migrate->src[i] & MIGRATE_PFN_MIGRATE))
>   				continue;
>   			if (!notified) {