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=-2.4 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 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 A0DB1C5DF61 for ; Thu, 7 Nov 2019 21:04:25 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 58DDA21D7B for ; Thu, 7 Nov 2019 21:04:25 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="AlyqBFL8" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 58DDA21D7B Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 064926B0003; Thu, 7 Nov 2019 16:04:25 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id F2FF66B0006; Thu, 7 Nov 2019 16:04:24 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E1F986B0007; Thu, 7 Nov 2019 16:04:24 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0124.hostedemail.com [216.40.44.124]) by kanga.kvack.org (Postfix) with ESMTP id CC20F6B0003 for ; Thu, 7 Nov 2019 16:04:24 -0500 (EST) Received: from smtpin19.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with SMTP id 80AA2824999B for ; Thu, 7 Nov 2019 21:04:24 +0000 (UTC) X-FDA: 76130709648.19.rat86_608b5768e8e0b X-HE-Tag: rat86_608b5768e8e0b X-Filterd-Recvd-Size: 7419 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-2.mimecast.com [205.139.110.61]) by imf26.hostedemail.com (Postfix) with ESMTP for ; Thu, 7 Nov 2019 21:04:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1573160662; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=+/OUHbhjqmo/2b88pcHR1Lly3F7Wmo3w1j9CIvENsK0=; b=AlyqBFL8tohyiQmqvpnHACApZbaYdxjTYX/X/h3AxdQr3qS2kpo9LS57bCZn9wMJaFrA5T IMar1/tqWtBibHmJjKZb+Wh7+P1UiMrNvTK6aGSKdzT2KevI0ajyFxHAnWIt8lyryFyNVt CR3z4K7exo8qGP1R6nFR5r4b3mcZ2XI= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-308-5mTC5mxRONi9JJNT1c4IYQ-1; Thu, 07 Nov 2019 16:04:19 -0500 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 08CED1005500; Thu, 7 Nov 2019 21:04:14 +0000 (UTC) Received: from redhat.com (ovpn-122-19.rdu2.redhat.com [10.10.122.19]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E81285C548; Thu, 7 Nov 2019 21:04:10 +0000 (UTC) Date: Thu, 7 Nov 2019 16:04:08 -0500 From: Jerome Glisse To: Jason Gunthorpe Cc: John Hubbard , "linux-mm@kvack.org" , Ralph Campbell , "Felix.Kuehling@amd.com" , "linux-rdma@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , "amd-gfx@lists.freedesktop.org" , Alex Deucher , Ben Skeggs , Boris Ostrovsky , Christian =?iso-8859-1?Q?K=F6nig?= , David Zhou , Dennis Dalessandro , Juergen Gross , Mike Marciniszyn , Oleksandr Andrushchenko , Petr Cvek , Stefano Stabellini , "nouveau@lists.freedesktop.org" , "xen-devel@lists.xenproject.org" , Christoph Hellwig , Andrea Arcangeli , Michal Hocko Subject: Re: [PATCH v2 02/15] mm/mmu_notifier: add an interval tree notifier Message-ID: <20191107210408.GA4716@redhat.com> References: <20191028201032.6352-1-jgg@ziepe.ca> <20191028201032.6352-3-jgg@ziepe.ca> <35c2b322-004e-0e18-87e4-1920dc71bfd5@nvidia.com> <20191107020807.GA747656@redhat.com> <20191107201102.GC21728@mellanox.com> MIME-Version: 1.0 In-Reply-To: <20191107201102.GC21728@mellanox.com> User-Agent: Mutt/1.12.1 (2019-06-15) X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-MC-Unique: 5mTC5mxRONi9JJNT1c4IYQ-1 X-Mimecast-Spam-Score: 0 Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable Content-Disposition: inline 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: On Thu, Nov 07, 2019 at 08:11:06PM +0000, Jason Gunthorpe wrote: > On Wed, Nov 06, 2019 at 09:08:07PM -0500, Jerome Glisse wrote: >=20 > > >=20 > > > Extra credit: IMHO, this clearly deserves to all be in a new mmu_rang= e_notifier.h > > > header file, but I know that's extra work. Maybe later as a follow-up= patch, > > > if anyone has the time. > >=20 > > The range notifier should get the event too, it would be a waste, i thi= nk it is > > an oversight here. The release event is fine so NAK to you separate eve= nt. Event > > is really an helper for notifier i had a set of patch for nouveau to le= verage > > this i need to resucite them. So no need to split thing, i would just f= orward > > the event ie add event to mmu_range_notifier_ops.invalidate() i failed = to catch > > that in v1 sorry. >=20 > I think what you mean is already done? >=20 > struct mmu_range_notifier_ops { > =09bool (*invalidate)(struct mmu_range_notifier *mrn, > =09=09=09 const struct mmu_notifier_range *range, > =09=09=09 unsigned long cur_seq); Yes it is sorry, i got confuse with mmu_range_notifier and mmu_notifier_ran= ge :) It is almost a palyndrome structure ;) >=20 > > No it is always odd, you must call mmu_range_set_seq() only from the > > op->invalidate_range() callback at which point the seq is odd. As well > > when mrn is added and its seq first set it is set to an odd value > > always. Maybe the comment, should read: > >=20 > > * mrn->invalidate_seq is always, yes always, set to an odd value. This= ensures > >=20 > > To stress that it is not an error. >=20 > I went with this: >=20 > =09/* > =09 * mrn->invalidate_seq must always be set to an odd value via > =09 * mmu_range_set_seq() using the provided cur_seq from > =09 * mn_itree_inv_start_range(). This ensures that if seq does wrap we > =09 * will always clear the below sleep in some reasonable time as > =09 * mmn_mm->invalidate_seq is even in the idle state. > =09 */ Yes fine with me. [...] > > > > +=09might_lock(&mm->mmap_sem); > > > > + > > > > +=09mmn_mm =3D smp_load_acquire(&mm->mmu_notifier_mm); > > >=20 > > > What does the above pair with? Should have a comment that specifies t= hat. > >=20 > > It was discussed in v1 but maybe a comment of what was said back then w= ould > > be helpful. Something like: > >=20 > > /* > > * We need to insure that all writes to mm->mmu_notifier_mm are visible= before > > * any checks we do on mmn_mm below as otherwise CPU might re-order wri= te done > > * by another CPU core to mm->mmu_notifier_mm structure fields after th= e read > > * belows. > > */ >=20 > This comment made it, just at the store side: >=20 > =09/* > =09 * Serialize the update against mmu_notifier_unregister. A > =09 * side note: mmu_notifier_release can't run concurrently with > =09 * us because we hold the mm_users pin (either implicitly as > =09 * current->mm or explicitly with get_task_mm() or similar). > =09 * We can't race against any other mmu notifier method either > =09 * thanks to mm_take_all_locks(). > =09 * > =09 * release semantics on the initialization of the mmu_notifier_mm's > * contents are provided for unlocked readers. acquire can only = be > * used while holding the mmgrab or mmget, and is safe because on= ce > * created the mmu_notififer_mm is not freed until the mm is > * destroyed. As above, users holding the mmap_sem or one of the > * mm_take_all_locks() do not need to use acquire semantics. > =09 */ > =09if (mmu_notifier_mm) > =09=09smp_store_release(&mm->mmu_notifier_mm, mmu_notifier_mm); >=20 > Which I think is really overly belaboring the typical smp > store/release pattern, but people do seem unfamiliar with them... Perfect with me. I think also sometimes you forgot what memory model is and thus store/release pattern do, i know i do and i need to refresh my mind. Cheers, J=E9r=F4me