Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Date:   Mon, 11 Dec 2023 17:00:50 -0800
From:   Dan Williams <dan.j.williams@intel.com>
To:     "Verma, Vishal L" <vishal.l.verma@intel.com>,
        "Huang, Ying" <ying.huang@intel.com>
CC:     "david@redhat.com" <david@redhat.com>,
        "Jiang, Dave" <dave.jiang@intel.com>,
        "dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
        "linux-cxl@vger.kernel.org" <linux-cxl@vger.kernel.org>,
        "Jonathan.Cameron@huawei.com" <Jonathan.Cameron@huawei.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Williams, Dan J" <dan.j.williams@intel.com>,
        "nvdimm@lists.linux.dev" <nvdimm@lists.linux.dev>,
        "lizhijian@fujitsu.com" <lizhijian@fujitsu.com>
Subject: Re: [PATCH v3 2/2] dax: add a sysfs knob to control memmap_on_memory
 behavior
Message-ID: <6577b0c2a02df_a04c5294bb@dwillia2-xfh.jf.intel.com.notmuch>
References: <20231211-vv-dax_abi-v3-0-acf6cc1bde9f@intel.com>
 <20231211-vv-dax_abi-v3-2-acf6cc1bde9f@intel.com>
 <87msugxnx9.fsf@yhuang6-desk2.ccr.corp.intel.com>
 <aac91f0ae8774c521469d518585a499da52912a8.camel@intel.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <aac91f0ae8774c521469d518585a499da52912a8.camel@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?Ngg1+YB3ddvq6mndiE5C18BjIUxNA1twXlzokqsRr1RxNueQdVrz/3nzC6?=
 =?iso-8859-1?Q?4ngZroy9HyFjy5qTuONkor27elsUC7OrZSpamb/7wDI0g3PpTNMgtU7r0S?=
 =?iso-8859-1?Q?EqeFg4It7M6vH89dF3gyU9Xz0HW2jx7kRavgPuuglfNWjzPVpayRjzyEUF?=
 =?iso-8859-1?Q?7DTu02PhjW4jNwSxLHLtv+vMY7vqyLL0vsLqG3uw6Fa2IKQxGEMp3fO+H+?=
 =?iso-8859-1?Q?jBA94LuYJLSLC5VFRBXbU0jAIFLSFi5ScGWBot3pdwqnORI+MguonoCAC7?=
 =?iso-8859-1?Q?/4WjE49wrWq6QHoUQ5oxRUFlmHUysveKMygtcTHPFYk8FBJ8cO/mDZVt61?=
 =?iso-8859-1?Q?pPdVq8Dv1ufOxpyT2btygUxRgkX8YqGwhWGO30jdRCKxyc4Z9jseS1Jao2?=
 =?iso-8859-1?Q?hLC5mk1iD+oH49GStMa70WqxOM52fJHQVxXs9zWMNFu+BirrY5tAhHqc0m?=
 =?iso-8859-1?Q?/g+kMhstUNH8Hzd5Kj7kYYubf23RpEhboUIFqWiABT9NaVj0BWF1UQG4C+?=
 =?iso-8859-1?Q?DC76MpCEs/z+PXnhih1I0Y+oJnkWRjhw5mcmxXsUapvYllfQyozatnnGKH?=
 =?iso-8859-1?Q?TpGYbDGQqrd3QLtd1rqrp/GZDh7jteuIEwZ2JasaZg2Yu5ga4uvk40mWbV?=
 =?iso-8859-1?Q?oK6AMxqotzBFyZ/0SK4ImWQDvFHveFv6i0iqXdnGBy+SlQLZ7UeULFzd8F?=
 =?iso-8859-1?Q?zfzJrcKvgCKpsSuWfK4xufMpTocOUA7rG/39lPBy6uUKNEsYXzoYgfP3vf?=
 =?iso-8859-1?Q?2YVDjOnhx2gng5VxySwyVmqw7OAkf3XMpGzYNr9CTbuZNApjTJFH5f+vhf?=
 =?iso-8859-1?Q?smWgg/W2JH5pT3h5LEHd4ts++EeqFE8o5fA2ItrCSqu6x5MNkwG5Q6t/W0?=
 =?iso-8859-1?Q?JL57Z8YrePTpGR0KDIwHxhTNDf6PMPYIU5nzd+FOx2YIND/9dOBYGT8mip?=
 =?iso-8859-1?Q?s6XNKgfonbWNU43hxW+MGP76icZeJLd2M3TQMyoMlBUi5VY1FqkJiiEh8/?=
 =?iso-8859-1?Q?73Z0Kv7gJM7LGdX/uyUp/kUpfURDd0W8VTU3mNeUra8Kd2Wurudd1sXqot?=
 =?iso-8859-1?Q?fdX9UKlYBt3x6DGidPktwxmZdR0DDHGyWUd2HKlL7jGGTRBHHU3KkY2Fi5?=
 =?iso-8859-1?Q?J7MnFpAZbmyvGbYqeq/5YbLvY72Rs75QW8qJvDRPVaUDpKxmZW24CZ35wz?=
 =?iso-8859-1?Q?/qhO4b8x6bnOhXjh8jZT3NSOHngd1fgVUF602WApey2IX0oNDbiLLOWFJZ?=
 =?iso-8859-1?Q?+p1gm+ja+LLAwl8x+N/OXEUzs5DJyp7JG7UwOstH7BcQDXp/4mFx4AVU/i?=
 =?iso-8859-1?Q?ffS5yBXrUIcP/gF3VRRexgXe4QxGcDHP/3eVDJ7eRI4t/n76CSE/gZcP/5?=
 =?iso-8859-1?Q?1+8shKCcIoLjXBy5cOSi8kWy/kfIJ5eqjhHYaGfTTYnqiqYjGXqSTmQcsK?=
 =?iso-8859-1?Q?W3QgWQJuGJ8RQl5kDSXSjV4+i+4hZ7DhLk1iXUt/fXqetET7fuWM8Czr1l?=
 =?iso-8859-1?Q?XrNtdNudsH4uKnC8/4dl/+5pUqxOmDb5nsjZofiXX+ktYddwK8/4vMICCT?=
 =?iso-8859-1?Q?juK22iMuAs4VXfkydF8gs5oPQ1LX7CQIUOnVc576iKkueZ72ADoM/NfIcP?=
 =?iso-8859-1?Q?7Y3OLtvhphUjzOppKMvowU82a/X77dJCbMHeO3lS5N7ttxZK2JVkHJGA?=
 =?iso-8859-1?Q?=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 11f9f861-a6b5-47fc-61fe-08dbfaadca5a
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Dec 2023 01:00:54.6050
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: CYgq9Z6b9cgBaPda7m6K5Gz9vtWmuwoMlfIPns6V2+aRvgjkuYCXgdNUJWYbD3UD8tBCYrbxSytVuBax7TAuyoucQXdtfd69tkA+yLvyC/0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB5191
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on howler.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Mon, 11 Dec 2023 17:01:12 -0800 (PST)

Verma, Vishal L wrote:
> On Tue, 2023-12-12 at 08:30 +0800, Huang, Ying wrote:
> > Vishal Verma <vishal.l.verma@intel.com> writes:
> > 
> > > Add a sysfs knob for dax devices to control the memmap_on_memory setting
> > > if the dax device were to be hotplugged as system memory.
> > > 
> > > The default memmap_on_memory setting for dax devices originating via
> > > pmem or hmem is set to 'false' - i.e. no memmap_on_memory semantics, to
> > > preserve legacy behavior. For dax devices via CXL, the default is on.
> > > The sysfs control allows the administrator to override the above
> > > defaults if needed.
> > > 
> > > Cc: David Hildenbrand <david@redhat.com>
> > > Cc: Dan Williams <dan.j.williams@intel.com>
> > > Cc: Dave Jiang <dave.jiang@intel.com>
> > > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > > Cc: Huang Ying <ying.huang@intel.com>
> > > Tested-by: Li Zhijian <lizhijian@fujitsu.com>
> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Reviewed-by: David Hildenbrand <david@redhat.com>
> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > > ---
> > > ?drivers/dax/bus.c?????????????????????? | 47 +++++++++++++++++++++++++++++++++
> > > ?Documentation/ABI/testing/sysfs-bus-dax | 17 ++++++++++++
> > > ?2 files changed, 64 insertions(+)
> > > 
> > > diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
> > > index 1ff1ab5fa105..2871e5188f0d 100644
> > > --- a/drivers/dax/bus.c
> > > +++ b/drivers/dax/bus.c
> > > @@ -1270,6 +1270,52 @@ static ssize_t numa_node_show(struct device *dev,
> > > ?}
> > > ?static DEVICE_ATTR_RO(numa_node);
> > > ?
> > > +static ssize_t memmap_on_memory_show(struct device *dev,
> > > +??????????????????????????????????? struct device_attribute *attr, char *buf)
> > > +{
> > > +???????struct dev_dax *dev_dax = to_dev_dax(dev);
> > > +
> > > +???????return sprintf(buf, "%d\n", dev_dax->memmap_on_memory);
> > > +}
> > > +
> > > +static ssize_t memmap_on_memory_store(struct device *dev,
> > > +???????????????????????????????????? struct device_attribute *attr,
> > > +???????????????????????????????????? const char *buf, size_t len)
> > > +{
> > > +???????struct device_driver *drv = dev->driver;
> > > +???????struct dev_dax *dev_dax = to_dev_dax(dev);
> > > +???????struct dax_region *dax_region = dev_dax->region;
> > > +???????struct dax_device_driver *dax_drv = to_dax_drv(drv);
> > > +???????ssize_t rc;
> > > +???????bool val;
> > > +
> > > +???????rc = kstrtobool(buf, &val);
> > > +???????if (rc)
> > > +???????????????return rc;
> > > +
> > > +???????if (dev_dax->memmap_on_memory == val)
> > > +???????????????return len;
> > > +
> > > +???????device_lock(dax_region->dev);
> > > +???????if (!dax_region->dev->driver) {
> > > +???????????????device_unlock(dax_region->dev);
> > > +???????????????return -ENXIO;
> > > +???????}
> > 
> > I think that it should be OK to write to "memmap_on_memory" if no driver
> > is bound to the device.? We just need to avoid to write to it when kmem
> > driver is bound.
> 
> Oh this is just a check on the region driver, not for a dax driver
> being bound to the device. It's the same as what things like
> align_store(), size_store() etc. do for dax device reconfiguration.
> 
> That said, it might be okay to remove this check, as this operation
> doesn't change any attributes of the dax region (the other interfaces I
> mentioned above can affect regions, so we want to lock the region
> device). If removing the check, we'd drop the region lock acquisition
> as well.
> 
> Dan, any thoughts on this?

Since this is a dev_dax attribute then this would have already been
synchronously shutdown when dax_region->dev->driver transitioned to
NULL. I.e. region unbind causes dev_dax deletion.

However, there's a different issue here as dev->driver was referenced
without the device_lock().

Additionally, I think this function also makes it clear that device lock
flavor of guard() would be useful:

    DEFINE_GUARD(dev, struct device *, device_lock(_T), device_unlock(_T))

...then I would expect something like:

        guard(dev)(dev);
        if (dev_dax->memmap_on_memory != val && dev->driver &&
            to_dax_drv(dev->driver)->type == DAXDRV_KMEM_TYPE)
                return -EBUSY;
        dev_dax->memmap_on_memory = val;
        return len;

...maybe with some temp variables to reduce the derefence chain, buy you
get the idea. Only prevent changes while the device is active under
kmem.