netdev - Re: [PATCH v2 2/2] vmalloc: Remove work as from vfree path

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4e353614f017c7c13a21d168992852dae1762aba.camel@intel.com>
Date:   Tue, 21 May 2019 16:51:52 +0000
From:   "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To:     "luto@...nel.org" <luto@...nel.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "mroos@...ux.ee" <mroos@...ux.ee>,
        "redgecombe.lkml@...il.com" <redgecombe.lkml@...il.com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "namit@...are.com" <namit@...are.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        "bp@...en8.de" <bp@...en8.de>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "sparclinux@...r.kernel.org" <sparclinux@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] vmalloc: Remove work as from vfree path

On Tue, 2019-05-21 at 09:17 -0700, Andy Lutomirski wrote:
> On Mon, May 20, 2019 at 4:39 PM Rick Edgecombe
> <rick.p.edgecombe@...el.com> wrote:
> > From: Rick Edgecombe <redgecombe.lkml@...il.com>
> > 
> > Calling vm_unmap_alias() in vm_remove_mappings() could potentially
> > be a
> > lot of work to do on a free operation. Simply flushing the TLB
> > instead of
> > the whole vm_unmap_alias() operation makes the frees faster and
> > pushes
> > the heavy work to happen on allocation where it would be more
> > expected.
> > In addition to the extra work, vm_unmap_alias() takes some locks
> > including
> > a long hold of vmap_purge_lock, which will make all other
> > VM_FLUSH_RESET_PERMS vfrees wait while the purge operation happens.
> > 
> > Lastly, page_address() can involve locking and lookups on some
> > configurations, so skip calling this by exiting out early when
> > !CONFIG_ARCH_HAS_SET_DIRECT_MAP.
> 
> Hmm.  I would have expected that the major cost of vm_unmap_aliases()
> would be the flush, and at least informing the code that the flush
> happened seems valuable.  So would guess that this patch is actually
> a
> loss in throughput.
> 
You are probably right about the flush taking the longest. The original
idea of using it was exactly to improve throughput by saving a flush.
However with vm_unmap_aliases() the flush will be over a larger range
than before for most arch's since it will likley span from the module
space to vmalloc. From poking around the sparc tlb flush history, I
guess the lazy purges used to be (still are?) a problem for them
because it would try to flush each page individually for some CPUs. Not
sure about all of the other architectures, but for any implementation
like that, using vm_unmap_alias() would turn an occasional long
operation into a more frequent one.

On x86, it shouldn't be a problem to use it. We already used to call
this function several times around a exec permission vfree. 

I guess its a tradeoff that depends on how fast large range TLB flushes
usually are compared to small ones. I am ok dropping it, if it doesn't
seem worth it.