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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id E09C7C61CE7 for ; Sat, 7 Jun 2025 23:31:31 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1EE4C6B0088; Sat, 7 Jun 2025 19:31:31 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 19EED6B0089; Sat, 7 Jun 2025 19:31:31 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0B56F6B008A; Sat, 7 Jun 2025 19:31:31 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id E07546B0088 for ; Sat, 7 Jun 2025 19:31:30 -0400 (EDT) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 5A9681CE0E7 for ; Sat, 7 Jun 2025 23:31:30 +0000 (UTC) X-FDA: 83530203540.08.2C48A5B Received: from mail-qt1-f175.google.com (mail-qt1-f175.google.com [209.85.160.175]) by imf27.hostedemail.com (Postfix) with ESMTP id 7328540011 for ; Sat, 7 Jun 2025 23:31:28 +0000 (UTC) Authentication-Results: imf27.hostedemail.com; dkim=pass header.d=soleen-com.20230601.gappssmtp.com header.s=20230601 header.b=G++oAkIk; spf=pass (imf27.hostedemail.com: domain of pasha.tatashin@soleen.com designates 209.85.160.175 as permitted sender) smtp.mailfrom=pasha.tatashin@soleen.com; dmarc=pass (policy=none) header.from=soleen.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1749339088; h=from:from:sender: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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=nWW4tFwFmqVS//sP6tRLMxB/a9EcACAW7cnbM0vY4Rg=; b=aTWjfVJnNQF9ZLtxj26W/7wa6GjxyYfF+tcIIEohPArRNuJSm7d/qgzVlgh87g9WDT0rq8 Az96DUOEIf5Ad20vz9v/MtDQ3K48WDuAWtKTyJEJK0+WCerZ1zTf5J/akfB5iUHTC0PIyh t4lntCvkWxPr9SVKC8XvIinvKQSoqJo= ARC-Authentication-Results: i=1; imf27.hostedemail.com; dkim=pass header.d=soleen-com.20230601.gappssmtp.com header.s=20230601 header.b=G++oAkIk; spf=pass (imf27.hostedemail.com: domain of pasha.tatashin@soleen.com designates 209.85.160.175 as permitted sender) smtp.mailfrom=pasha.tatashin@soleen.com; dmarc=pass (policy=none) header.from=soleen.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1749339088; a=rsa-sha256; cv=none; b=gM32N7nCp5XS6P1l1ANVS/+5Y/OTKhChrRmPpzoQ9UzqPDZE32/2WDrDw5fPsIlCJwF/9D 2dpG+CFhFcle/cBuQ//DtEMiSDqSyEe7/PIltGX1lLg5cdeM2+kGWkKP/YIjC6ZCzHBu/7 +ZQncNKgxz+wZ9LdyB3+PPHRlcG549M= Received: by mail-qt1-f175.google.com with SMTP id d75a77b69052e-4a58ba6c945so52584241cf.2 for ; Sat, 07 Jun 2025 16:31:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=soleen-com.20230601.gappssmtp.com; s=20230601; t=1749339087; x=1749943887; darn=kvack.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=nWW4tFwFmqVS//sP6tRLMxB/a9EcACAW7cnbM0vY4Rg=; b=G++oAkIk5QXzm8RhCnp9MVJLZoIhpKvFdf5Y7ifFb1/rJKN3VqYZOBE6ra/vidSgSW F3dRjO43A4oFI1sOS+jpXUJ0pMKK5es2/X+USnE1nbX6+8opR87nkmdym3IYrXfLJ8Sn sQz2JIInAoxTU0LoitTKQL/jI4VHQxeXuioTmAMRszGkZuyzOPXSIlbltwhaQyjlSMk6 PRcRX3O67aK3iZ+xBSNn3rzNAy4j7jFg1faawkEGB5Ls2MqZdX1QngORy7FqwJ9znw1S coIGZwTb5jDXzbNS4aSwqucDeFQlf0YX/j9InEz6JXEUjrAdEsPzpOCsH7YwXDh5u4Uu s4/Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1749339087; x=1749943887; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=nWW4tFwFmqVS//sP6tRLMxB/a9EcACAW7cnbM0vY4Rg=; b=LW0MbQkA4A3MMWv+raq4cAGwA85UWHXgereAmFuU7YuaMDFG/4j/QxsUsl/oXlOe2n xrls2p/tSrNY49Jm955jGX9CKJtg86W4jd7I6f9U2BzMNrybF3WGCUZcFvdO5LFBzerO J5odzKwhVrHKwcu8OLAHlENrLtK4hktqaZHYKbLyo3j7/ojjrF5yCg4Jd+K5aQX8NA92 e2Zr+IgnFlexewLa1HdqNHaSufdHQKK7L8Ac6Plpn6AH4vn+D7PeRd22P9glM8+OB1Ty uhyuWGOgm6cJ6fqA1qmLroYm4zZXzl3t3Ap7Rfk0nSKTlNc09bB0WuoXr+nroG4M2Mbs kIgw== X-Forwarded-Encrypted: i=1; AJvYcCWTt7EBOrYGs0E5VoLBEG1BecDwQY4sHz8X3nxnQMxUAcOOE+L4I0ppoLLVtwaw4RCOyMKRbz/YXQ==@kvack.org X-Gm-Message-State: AOJu0Yz5Drp4h4HgvZCgSN7PvbBn4VAPLWZNUn1LnUjT0+AP5IfKuMii L95sjxRJCNCtE+jIPd9yWO9F8D3ql9C6qPiEHrh4Wz87XNwvp8eG/yrTSLGbgFPKmSiqIDE83ok W+B+UGFcdYExCDO1yH9UhP7cQKK/pYhXlKUI6OWCobQ== X-Gm-Gg: ASbGncslaGsSW4IUoKPQ/CAluHQkiEmG9cU/btZuWHkd7PSqkp6FxFqCb79Pkx7moYQ PHBS8J6McOPwNa1KkP+vjjk7SX08U2NEeNF5rwRATOxkaOStSPvWWs2ZWe84eOeCKMQfkUMDj4B 9t383IaNVz8ieI0eMe+z5AyNaNISr61GjpkrWyytHh X-Google-Smtp-Source: AGHT+IE/jhKXlDr4jL3M75mzvowzE14RL3PEF2Uct9qhaEWX17FT27TRjOroOxAGSkk1IHz01pymOxp4I9oUtrJerjw= X-Received: by 2002:a05:622a:1f8a:b0:4a4:3f02:da8a with SMTP id d75a77b69052e-4a5b9a04abbmr157361561cf.1.1749339087430; Sat, 07 Jun 2025 16:31:27 -0700 (PDT) MIME-Version: 1.0 References: <20250515182322.117840-1-pasha.tatashin@soleen.com> <20250515182322.117840-6-pasha.tatashin@soleen.com> In-Reply-To: From: Pasha Tatashin Date: Sat, 7 Jun 2025 19:30:50 -0400 X-Gm-Features: AX0GCFs-BjRusZNGHsxVWMv-Mr6Y0p0TY9yHb7099prmZv0f_v8HXx6iETmf-Tg Message-ID: Subject: Re: [RFC v2 05/16] luo: luo_core: integrate with KHO To: Pratyush Yadav Cc: jasonmiu@google.com, graf@amazon.com, changyuanl@google.com, rppt@kernel.org, dmatlack@google.com, rientjes@google.com, corbet@lwn.net, rdunlap@infradead.org, ilpo.jarvinen@linux.intel.com, kanie@linux.alibaba.com, ojeda@kernel.org, aliceryhl@google.com, masahiroy@kernel.org, akpm@linux-foundation.org, tj@kernel.org, yoann.congal@smile.fr, mmaurer@google.com, roman.gushchin@linux.dev, chenridong@huawei.com, axboe@kernel.dk, mark.rutland@arm.com, jannh@google.com, vincent.guittot@linaro.org, hannes@cmpxchg.org, dan.j.williams@intel.com, david@redhat.com, joel.granados@kernel.org, rostedt@goodmis.org, anna.schumaker@oracle.com, song@kernel.org, zhangguopeng@kylinos.cn, linux@weissschuh.net, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, linux-mm@kvack.org, gregkh@linuxfoundation.org, tglx@linutronix.de, mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com, x86@kernel.org, hpa@zytor.com, rafael@kernel.org, dakr@kernel.org, bartosz.golaszewski@linaro.org, cw00.choi@samsung.com, myungjoo.ham@samsung.com, yesanishhere@gmail.com, Jonathan.Cameron@huawei.com, quic_zijuhu@quicinc.com, aleksander.lobakin@intel.com, ira.weiny@intel.com, andriy.shevchenko@linux.intel.com, leon@kernel.org, lukas@wunner.de, bhelgaas@google.com, wagi@kernel.org, djeffery@redhat.com, stuart.w.hayes@gmail.com Content-Type: text/plain; charset="UTF-8" X-Rspam-User: X-Rspamd-Queue-Id: 7328540011 X-Rspamd-Server: rspam09 X-Stat-Signature: 6ht6n3cgft7uxdgi9mx6pua3j6qek1c3 X-HE-Tag: 1749339088-886722 X-HE-Meta: U2FsdGVkX18mN9X2ZbkM/15aAUpqOVdMLe3u+aO5sB3/5oHhfkVa1/TzVlm+oAyQk/h2JoxIY6x4bKRh/S1iUc59XLhPZEY/gTIiztdr9hKO+yKX1XkADad8biXZv0WDp2biiCezqhrqhU1Q1Zr3temKUaMvYr/Jr2nj6kJEBkPdbkVfURztSho3RPS/4j6PtaaM3rxRafyNM1v1Ov8DOPkd/xPOlrRodQZHd1aTNgD9qtacRIB3y4RBHnU+LgMJijfOFexOe3FtGS1HHtIvMqTp7KdxQFpbZhFVJjcGX06a/lfHxHDcqoqe5fD76tRYSKamhm8Yn++zF/YfWwvTrR8Oo27HBtEUCu0CdS5CQ7FmpCuqylwdTj9nw0TZ2X1wwCdpkE3vE6xJb4SbZx70SD5F8mfKt0PgTvTZOIh9XtS9kxkPF642iqFGOJDEJ6w920Wn0Pz8nJVPHb7IBG/fbgljzym6vCiUawKf8yPpGDlRgq8mDOwpPRCJbVqRAPTpFsaEHi2+blPhHf3KNtquoigLps3ks5IzE1tZ1Zu+jdNuxpwJQT2fMl1p+wQ4Nh++h4qoNZhGVSrqWFKoJPoXr+Z//tlTfyPJxdDyoZyAbs+cRszCqTLizECmtYqQJuMULThASGx8aC7zGri9/v3UAF2gcO7gu4umXmLB/xWHkrtc0BNOnzfVcJwsr2EMtyq2tjLRKJyfND0wRjCplLPIdogqa64sjkNXxfI9Klgd/+WVXyb+AfWCk5w//dvCLak/8E5vXyWfaEhcg7Pd12bzRiJQl09MAyv9SxTOD0oxkUDt9FT8R+q5G6jert28095EHMqkcFYmmdcOkPVogfUZ16odbtUuZHM5NBC3/gPu+akmCqKwAPmalqVyj++yzyrqFF945HmGcszHrYf/PotyE2mAqxX/aqQTY0UAK3IJ2njG1gbYIKQRiagZTZA6NMurX7TTARqrHSLZwwP5jz3 r8O0ehoU WZpKSMvfGf4Wm+Xr4hD2Pvzyg4OGWyiY/jfYgwq+A64AfwYrYGLa+HogUr1TRWG51bwpdfX+iJMxHWHfHoiP7j9NDzyGXw7w5pB+uUZKKqEJD/7vMLYC6KBybIkK4+Pl1NjkW0UDHyeSc8eExOcyj8j+GY3gxeyF/3Y1urojy153WNJD14yW2HKo4xCy+J//EeGbhYXeeI/dnDXLrixrmxgUa2S9e0dV1xsseRZRIPTjsXnLt+uOPS2RPpukzKkIvQ1DTnRbosm0itwkYrw9SJOjK3dmHTncA/c/C5PTfTgytqgrx5iy48EnpuegKjdU4k9Df9eHOsmFSfCrTE5oq7ZVnJcp8PnPdgNFYTiBuMdD1tbWzDyEVCS9XZg== 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: List-Subscribe: List-Unsubscribe: > > + fdt_out = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, > > + get_order(LUO_FDT_SIZE)); > > Why not alloc_folio()? KHO already deals with folios so it seems > simpler. The kho_{un,}preserve_folio() functions exist exactly for these > kinds of allocations, so LUO also ends up being a first user. You also > won't end up needing kho_unpreserve_phys() and all the __pa() calls. I prefer phys here, because this way, we are not bound for size and alignment to be of a specific order, it can be n-pages instead. > > + if (!fdt_out) { > > + pr_err("failed to allocate FDT memory\n"); > > + return -ENOMEM; > > + } > > + > > + ret = fdt_create_empty_tree(fdt_out, LUO_FDT_SIZE); > > You are using FDT read/write functions throughout the series to create > new FDTs. The sequential write functions are generally more efficient > since they are meant for creating new FDT blobs. The read/write > functions are better for modifying an existing FDT blob. > > Is there a particular reason you do this? > > When using FDT SW functions, the creation of the tree would be something > like: > > fdt_create() > fdt_finish_reservemap() > fdt_begin_node() > > // Add stuff to FDT > > fdt_end_node() > fdt_finish() > > In this patch, the FDT does not change much after creation so it doesn't > look like it matters much, but in later patches, the FDT is passed to > luo_files_fdt_setup() and luo_subsystems_fdt_setup() which probably > modify the FDT a fair bit. The number of changes to empty tree FDT is small, and this is done only once, so I do think the extra cost is substantial. This could be a future optimization. Also, we could use a hybird approach where luo_files/luo_subsystems do the SW updates, while here we do Read/Write updates. > > + if (ret) > > + goto exit_free; > > + > > + ret = fdt_setprop(fdt_out, 0, "compatible", LUO_COMPATIBLE, > > + strlen(LUO_COMPATIBLE) + 1); > > fdt_setprop_string() instead? Or if you change to FDT SW, Updated, thanks! > fdt_property_string(). > > > + if (ret) > > + goto exit_free; > > + > > + ret = kho_preserve_phys(__pa(fdt_out), LUO_FDT_SIZE); > > + if (ret) > > + goto exit_free; > > + > > + ret = kho_add_subtree(ser, LUO_KHO_ENTRY_NAME, fdt_out); > > + if (ret) > > + goto exit_unpreserve; > > + luo_fdt_out = fdt_out; > > + > > + return 0; > > + > > +exit_unpreserve: > > + kho_unpreserve_phys(__pa(fdt_out), LUO_FDT_SIZE); > > +exit_free: > > + free_pages((unsigned long)fdt_out, get_order(LUO_FDT_SIZE)); > > + pr_err("failed to prepare LUO FDT: %d\n", ret); > > + > > + return ret; > > +} > > + > > +static void luo_fdt_destroy(void) > > +{ > > + kho_unpreserve_phys(__pa(luo_fdt_out), LUO_FDT_SIZE); > > + free_pages((unsigned long)luo_fdt_out, get_order(LUO_FDT_SIZE)); > > + luo_fdt_out = NULL; > > +} > > + > > +static int luo_do_prepare_calls(void) > > +{ > > + return 0; > > +} > > + > > static int luo_do_freeze_calls(void) > > { > > return 0; > > @@ -88,11 +151,111 @@ static void luo_do_finish_calls(void) > > { > > } > > > > -int luo_prepare(void) > > +static void luo_do_cancel_calls(void) > > +{ > > +} > > + > > +static int __luo_prepare(struct kho_serialization *ser) > > { > > + int ret; > > + > > + if (down_write_killable(&luo_state_rwsem)) { > > + pr_warn("[prepare] event canceled by user\n"); > > + return -EAGAIN; > > + } > > + > > + if (!is_current_luo_state(LIVEUPDATE_STATE_NORMAL)) { > > + pr_warn("Can't switch to [%s] from [%s] state\n", > > + luo_state_str[LIVEUPDATE_STATE_PREPARED], > > + LUO_STATE_STR); > > + ret = -EINVAL; > > + goto exit_unlock; > > + } > > + > > + ret = luo_fdt_setup(ser); > > + if (ret) > > + goto exit_unlock; > > + > > + ret = luo_do_prepare_calls(); > > + if (ret) > > + goto exit_unlock; > > With subsystems/filesystems support in place, this can fail. But since > luo_fdt_setup() called kho_add_subtree(), the debugfs file stays around, > and later calls to __luo_prepare() fail because the next > kho_add_subtree() tries to create a debugfs file that already exists. So > you would see an error like below: > > [ 767.339920] debugfs: File 'LUO' in directory 'sub_fdts' already present! > [ 767.340613] luo_core: failed to prepare LUO FDT: -17 > [ 767.341071] KHO: Failed to convert KHO state tree: -17 > [ 767.341593] luo_core: Can't switch to [normal] from [normal] state > [ 767.342175] KHO: Failed to abort KHO finalization: -22 > > You probably need a kho_remove_subtree() that can be called from the > error paths. > Note that __luo_cancel() is called because failure in a KHO finalize > notifier calls the abort notifiers. > > This is also something to fix, since if prepare fails, all other KHO > users who are already serialized won't even get to abort. Thank you for reporting this. This should not be happening, because if __luo_prepare() fails, the kho_abort should follow, however, KHO does not do kho_out_update_debugfs_fdt() when kho_finalize() fails, so I added this callback and it fixes this problem. I also added a selftest case for this. > > This weirdness happens because luo_prepare() and luo_cancel() control > the KHO state machine, but then also get controlled by it via the > notifier callbacks. So the relationship between then is not clear. > __luo_prepare() at least needs access to struct kho_serialization, so it > needs to come from the callback. So I don't have a clear way to clean > this all up off the top of my head. On production machine, without KHO_DEBUGFS, only LUO can control KHO state, but if debugfs is enabled, KHO can be finalized manually, and in this case LUO transitions to prepared state. In both cases, the path is identical. The KHO debugfs path is only for developers/debugging purposes. > > static int __init luo_startup(void) > > { > > - __luo_set_state(LIVEUPDATE_STATE_NORMAL); > > + phys_addr_t fdt_phys; > > + int ret; > > + > > + if (!kho_is_enabled()) { > > + if (luo_enabled) > > + pr_warn("Disabling liveupdate because KHO is disabled\n"); > > + luo_enabled = false; > > + return 0; > > + } > > + > > + ret = register_kho_notifier(&luo_kho_notifier_nb); > > + if (ret) { > > + luo_enabled = false; > > You set luo_enabled to false here, but none of the LUO entry points like > luo_prepare() or luo_freeze() actually check it. So LUO will appear work > just fine even when it hasn't initialized properly. luo_enabled check was missing from luo_ioctl.c, as we should not create a device if LUO is not enabled. This is fixed. > > > + pr_warn("Failed to register with KHO [%d]\n", ret); > > I guess you don't return here so a previous liveupdate can still be > recovered, even though we won't be able to make the next one. If so, a > comment would be nice to point this out. This is correct, but this is not going to work. Because, with the current change I am disabling "/dev/liveupdate" iff luo_enable == false. Let's just return here, failing to register with KHO should not really happen, it usually means that there is another notifier with the same name has already registered.