From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.4 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6E67BC3B1BD for ; Fri, 14 Feb 2020 18:47:13 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 228DD20848 for ; Fri, 14 Feb 2020 18:47:13 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="Q0XhyH6X" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 228DD20848 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id C5FB06B0668; Fri, 14 Feb 2020 13:47:12 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id C0F2C6B0669; Fri, 14 Feb 2020 13:47:12 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id AD7796B066A; Fri, 14 Feb 2020 13:47:12 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0108.hostedemail.com [216.40.44.108]) by kanga.kvack.org (Postfix) with ESMTP id 923326B0668 for ; Fri, 14 Feb 2020 13:47:12 -0500 (EST) Received: from smtpin28.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 1C5C3180AD817 for ; Fri, 14 Feb 2020 18:47:12 +0000 (UTC) X-FDA: 76489615104.28.brush51_49675449075d X-HE-Tag: brush51_49675449075d X-Filterd-Recvd-Size: 6697 Received: from mail-ed1-f48.google.com (mail-ed1-f48.google.com [209.85.208.48]) by imf35.hostedemail.com (Postfix) with ESMTP for ; Fri, 14 Feb 2020 18:47:11 +0000 (UTC) Received: by mail-ed1-f48.google.com with SMTP id p23so12314478edr.5 for ; Fri, 14 Feb 2020 10:47:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=SiWqUpdh4i6ijlaIGQ6h9I93/rzJCmHhBbrF7O5i8NM=; b=Q0XhyH6Xlhrpl2gwB631HswEjt4oCyWy3lLVptoDwWxl0HG3Q3C88cYuIRC4OBPf6w auhtMnSkhu8J6PeB+HyjnZFro/At+yFVhwPKlWilHic9FKPWhKmFaP4DkfrtfAuum9mS LUqDX+lJPCC892vzT6mJ5j1dI9Xpb7ZjQU85WsW6rnIZ4F4jbixPzQ6fZYsdSYa/0g53 jUmfiKCGOYHTOgfi/GWQ02YtcPM5tphrucdmwLiHMuMdGPgTQCfhlIT5CyRdNbli20Sh T7HcP9jyAHwBdOPmJdubumHSDbsffx622LiG2f5QAwsrbLB7JjhMhSLPs6tG9rngpZjJ O3gg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=SiWqUpdh4i6ijlaIGQ6h9I93/rzJCmHhBbrF7O5i8NM=; b=p3k84vvo0oav3DMyIifADjgyseP1XWRuBYelcgYTnZ26QXPxlEgprHX8thfHlh7dv6 hIk1F7ko50CzTtebONbxgOXUU1BGJKSelQtIruwX+qMb0zdSpExRvsrYlbTtj1+sOB68 +3YG64mvetabkPyvUAI4GTVk6YetYP6TS4dFWwOeY4IoLz6ChgGzptgdvzW9En+vlGiw IXu8ltrixlQyh0UI+B5PXD7aFp7mc/S8uJ6/UfTD9pbHYfcLdBkY5H0opJC2YGyVkXaq kwfXnnqTTATyFroOhJpCeT13rKQJUXh5725q8siPP7BycX9nwgeA66Cd0ruku4vWriV5 f6AA== X-Gm-Message-State: APjAAAUbnT8/K7SSGmGOqi7CXs+6IxcHABhokMeVeeSpcQkmQrbBqzBl oc4+QwtY6aF/VgTWyuFjhyn3iuaD13N8q71rg8Jmgg== X-Google-Smtp-Source: APXvYqzmUmMjxVo5zkPNo6yTS3RZoS0fNS3ryC6PdeYOUMtGdiDcZrTLR/xmBbN/aHCEcd4ePB3zkA4ob6/2yQnLqns= X-Received: by 2002:a05:6402:61a:: with SMTP id n26mr3817911edv.135.1581706029817; Fri, 14 Feb 2020 10:47:09 -0800 (PST) MIME-Version: 1.0 References: <20200207201856.46070-1-bgeffon@google.com> <20200214040952.43195-1-bgeffon@google.com> <20200214142857.kcmjiequhfl3sot2@box> In-Reply-To: <20200214142857.kcmjiequhfl3sot2@box> From: Brian Geffon Date: Fri, 14 Feb 2020 10:46:43 -0800 Message-ID: Subject: Re: [PATCH v5 1/2] mm: Add MREMAP_DONTUNMAP to mremap(). To: "Kirill A. Shutemov" Cc: Andrew Morton , "Michael S . Tsirkin" , Arnd Bergmann , LKML , linux-mm , Linux API , Andy Lutomirski , Will Deacon , Andrea Arcangeli , Sonny Rao , Minchan Kim , Joel Fernandes , Yu Zhao , Jesse Barnes , Nathan Chancellor , Florian Weimer Content-Type: text/plain; charset="UTF-8" X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: Hi Kirill, > > - if (vm_flags & VM_LOCKED) { > > - mm->locked_vm += new_len >> PAGE_SHIFT; > > - *locked = true; > > - } > > - > > Ah. You moved this piece. Why? Because we're not unmapping, do_munmap would have adjusted mm->locked_vm by decreasing it by old_len so then we have to add back in the new_len in the normal case (non. MREMAP_DONTUNMAP), but since we're not doing the unmap I want to skip the increase by new_len and just adjust accordingly. In the MREMAP_DONTUNMAP case if the VMA got smaller then the do_munmap on that portion would have decreased it by new_len - old_len, and the accounting is correct. In the case of an unchanged VMA size there is nothing to do, but in the case where it grows we're responsible for adding new_len - old_len because of the decision to jump that block and now the accounting is right for all cases. If we were to leave the original block and not jump over it then we would have to remove old_len bytes and then we're doing the same thing but now special casing the situation where new_len < old_len because the unmap on the removed part would have reduced it by new_len - old_len so backing old_len would be too much and we'd have to add back in new_len - old_len. I hope that explains it all. By doing it this way, IMO it makes it easier to see how the locked_vm accounting is happening because the vm_locked incrementing happens in only one of two places based on the type of remap that is happening. But I definitely can clean up the code a bit to drop the levels of indentation, maybe this: /* * locked_vm accounting: if the mapping remained the same size * it will have just moved and we don't need to touch locked_vm * because we skip the do_unmap. If the mapping shrunk before * being moved then the do_unmap on that portion will have * adjusted vm_locked. Only if the mapping grows do we need to * do something special; the reason is locked_vm only accounts * for old_len, but we're now adding new_len - old_len locked * bytes to the new mapping. */ if (vm_flags & VM_LOCKED && new_len > old_len) { mm->locked_vm += (new_len - old_len) >> PAGE_SHIFT; *locked = true; } /* We always clear VM_LOCKED[ONFAULT] on the old vma */ vma->vm_flags &= VM_LOCKED_CLEAR_MASK; goto out; } Having only one place where locked_vm is accounted and adjusted based on the type of remap seems like it will be easier to follow and less error prone later. What do you think about this? > > + if (flags & MREMAP_FIXED) { > > I think it has to be > > if (!(flags & MREMAP_DONTUNMAP)) { > > No? No. Because we dropped the requirement to use MREMAP_FIXED with MREMAP_DONTUNMAP, if we're not using MREMAP_FIXED we don't need to unmap anything at dest if it already exists because get_unmapped_area() below will not be using the MAP_FIXED flag either, instead it will search for a new unmapped area. If we were to change it then we wouldn't be able to do MREMAP_FIXED | MREMAP_DONTUNMAP, so I think this is correct. > > - if (flags & MREMAP_FIXED) { > > + if (flags & MREMAP_FIXED || flags & MREMAP_DONTUNMAP) { > > if (flags & (MREMAP_FIXED | MREMAP_DONTUNMAP)) { Sure, I can change that. If you're good with all of that I can mail a new patch today. Thanks again, Brian