On Wed, Aug 2, 2023 at 10:47 AM Michael Kelley (LINUX) <mikelley@xxxxxxxxxxxxx> wrote: > > From: Mitchell Levy via B4 Relay <devnull+levymitchell0.gmail.com@xxxxxxxxxx> Sent: Tuesday, July 25, 2023 5:24 PM > > > > This patch is intended as a proof-of-concept for the new SBRM > > machinery[1]. For some brief background, the idea behind SBRM is using > > the __cleanup__ attribute to automatically unlock locks (or otherwise > > release resources) when they go out of scope, similar to C++ style RAII. > > This promises some benefits such as making code simpler (particularly > > where you have lots of goto fail; type constructs) as well as reducing > > the surface area for certain kinds of bugs. > > > > The changes in this patch should not result in any difference in how the > > code actually runs (i.e., it's purely an exercise in this new syntax > > sugar). In one instance SBRM was not appropriate, so I left that part > > alone, but all other locking/unlocking is handled automatically in this > > patch. > > > > Link: https://lore.kernel.org/all/20230626125726.GU4253@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ [1] > > I haven't previously seen the "[1]" footnote-style identifier used with the > Link: tag. Usually the "[1]" goes at the beginning of the line with the > additional information, but that conflicts with the Link: tag. Maybe I'm > wrong, but you might either omit the footnote-style identifier, or the Link: > tag, instead of trying to use them together. Will be sure to fix this (along with the other formatting issues raised by you and Boqun) in a v2. > > Separately, have you built a kernel for ARM64 with these changes in > place? The Hyper-V balloon driver is used on both x86 and ARM64 > architectures. There's nothing obviously architecture specific here, > but given that SBRM is new, it might be wise to verify that all is good > when building and running on ARM64. I have built the kernel and confirmed that it's bootable on ARM64. I also disassembled the hv_balloon.o output by clang and GCC and compared the result to the disassembly of the pre-patch version. As far as I can tell, all the changes should be non-functional (some register renaming and flipping comparison instructions, etc.), but I don't believe I can thoroughly test at the moment as memory hot-add is disabled on ARM64. > > > > > Suggested-by: Boqun Feng <boqun.feng@xxxxxxxxx> > > Signed-off-by: "Mitchell Levy (Microsoft)" <levymitchell0@xxxxxxxxx> > > --- > > drivers/hv/hv_balloon.c | 82 +++++++++++++++++++++++-------------------------- > > 1 file changed, 38 insertions(+), 44 deletions(-) > > > > diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c > > index dffcc894f117..2812601e84da 100644 > > --- a/drivers/hv/hv_balloon.c > > +++ b/drivers/hv/hv_balloon.c > > @@ -8,6 +8,7 @@ > > > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > > > +#include <linux/cleanup.h> > > #include <linux/kernel.h> > > #include <linux/jiffies.h> > > #include <linux/mman.h> > > @@ -646,7 +647,7 @@ static int hv_memory_notifier(struct notifier_block *nb, unsigned long val, > > void *v) > > { > > struct memory_notify *mem = (struct memory_notify *)v; > > - unsigned long flags, pfn_count; > > + unsigned long pfn_count; > > > > switch (val) { > > case MEM_ONLINE: > > @@ -655,21 +656,22 @@ static int hv_memory_notifier(struct notifier_block *nb, unsigned long val, > > break; > > > > case MEM_OFFLINE: > > - spin_lock_irqsave(&dm_device.ha_lock, flags); > > - pfn_count = hv_page_offline_check(mem->start_pfn, > > - mem->nr_pages); > > - if (pfn_count <= dm_device.num_pages_onlined) { > > - dm_device.num_pages_onlined -= pfn_count; > > - } else { > > - /* > > - * We're offlining more pages than we managed to online. > > - * This is unexpected. In any case don't let > > - * num_pages_onlined wrap around zero. > > - */ > > - WARN_ON_ONCE(1); > > - dm_device.num_pages_onlined = 0; > > + scoped_guard(spinlock_irqsave, &dm_device.ha_lock) { > > + pfn_count = hv_page_offline_check(mem->start_pfn, > > + mem->nr_pages); > > + if (pfn_count <= dm_device.num_pages_onlined) { > > + dm_device.num_pages_onlined -= pfn_count; > > + } else { > > + /* > > + * We're offlining more pages than we > > + * managed to online. This is > > + * unexpected. In any case don't let > > + * num_pages_onlined wrap around zero. > > + */ > > + WARN_ON_ONCE(1); > > + dm_device.num_pages_onlined = 0; > > + } > > } > > - spin_unlock_irqrestore(&dm_device.ha_lock, flags); > > break; > > case MEM_GOING_ONLINE: > > case MEM_GOING_OFFLINE: > > @@ -721,24 +723,23 @@ static void hv_mem_hot_add(unsigned long start, unsigned long size, > > unsigned long start_pfn; > > unsigned long processed_pfn; > > unsigned long total_pfn = pfn_count; > > - unsigned long flags; > > > > for (i = 0; i < (size/HA_CHUNK); i++) { > > start_pfn = start + (i * HA_CHUNK); > > > > - spin_lock_irqsave(&dm_device.ha_lock, flags); > > - has->ha_end_pfn += HA_CHUNK; > > + scoped_guard(spinlock_irqsave, &dm_device.ha_lock) { > > + has->ha_end_pfn += HA_CHUNK; > > > > - if (total_pfn > HA_CHUNK) { > > - processed_pfn = HA_CHUNK; > > - total_pfn -= HA_CHUNK; > > - } else { > > - processed_pfn = total_pfn; > > - total_pfn = 0; > > - } > > + if (total_pfn > HA_CHUNK) { > > + processed_pfn = HA_CHUNK; > > + total_pfn -= HA_CHUNK; > > + } else { > > + processed_pfn = total_pfn; > > + total_pfn = 0; > > + } > > > > - has->covered_end_pfn += processed_pfn; > > - spin_unlock_irqrestore(&dm_device.ha_lock, flags); > > + has->covered_end_pfn += processed_pfn; > > + } > > > > reinit_completion(&dm_device.ol_waitevent); > > > > @@ -758,10 +759,10 @@ static void hv_mem_hot_add(unsigned long start, unsigned long size, > > */ > > do_hot_add = false; > > } > > - spin_lock_irqsave(&dm_device.ha_lock, flags); > > - has->ha_end_pfn -= HA_CHUNK; > > - has->covered_end_pfn -= processed_pfn; > > - spin_unlock_irqrestore(&dm_device.ha_lock, flags); > > + scoped_guard(spinlock_irqsave, &dm_device.ha_lock) { > > + has->ha_end_pfn -= HA_CHUNK; > > + has->covered_end_pfn -= processed_pfn; > > + } > > break; > > } > > > > @@ -781,10 +782,9 @@ static void hv_mem_hot_add(unsigned long start, unsigned long size, > > static void hv_online_page(struct page *pg, unsigned int order) > > { > > struct hv_hotadd_state *has; > > - unsigned long flags; > > unsigned long pfn = page_to_pfn(pg); > > > > - spin_lock_irqsave(&dm_device.ha_lock, flags); > > + guard(spinlock_irqsave)(&dm_device.ha_lock); > > list_for_each_entry(has, &dm_device.ha_region_list, list) { > > /* The page belongs to a different HAS. */ > > if ((pfn < has->start_pfn) || > > @@ -794,7 +794,6 @@ static void hv_online_page(struct page *pg, unsigned int order) > > hv_bring_pgs_online(has, pfn, 1UL << order); > > break; > > } > > - spin_unlock_irqrestore(&dm_device.ha_lock, flags); > > } > > > > static int pfn_covered(unsigned long start_pfn, unsigned long pfn_cnt) > > @@ -803,9 +802,8 @@ static int pfn_covered(unsigned long start_pfn, unsigned long pfn_cnt) > > struct hv_hotadd_gap *gap; > > unsigned long residual, new_inc; > > int ret = 0; > > - unsigned long flags; > > > > - spin_lock_irqsave(&dm_device.ha_lock, flags); > > + guard(spinlock_irqsave)(&dm_device.ha_lock); > > list_for_each_entry(has, &dm_device.ha_region_list, list) { > > /* > > * If the pfn range we are dealing with is not in the current > > @@ -852,7 +850,6 @@ static int pfn_covered(unsigned long start_pfn, unsigned long pfn_cnt) > > ret = 1; > > break; > > } > > - spin_unlock_irqrestore(&dm_device.ha_lock, flags); > > > > return ret; > > } > > @@ -947,7 +944,6 @@ static unsigned long process_hot_add(unsigned long pg_start, > > { > > struct hv_hotadd_state *ha_region = NULL; > > int covered; > > - unsigned long flags; > > > > if (pfn_cnt == 0) > > return 0; > > @@ -979,9 +975,9 @@ static unsigned long process_hot_add(unsigned long pg_start, > > ha_region->covered_end_pfn = pg_start; > > ha_region->end_pfn = rg_start + rg_size; > > > > - spin_lock_irqsave(&dm_device.ha_lock, flags); > > - list_add_tail(&ha_region->list, &dm_device.ha_region_list); > > - spin_unlock_irqrestore(&dm_device.ha_lock, flags); > > + scoped_guard(spinlock_irqsave, &dm_device.ha_lock) { > > + list_add_tail(&ha_region->list, &dm_device.ha_region_list); > > + } > > } > > > > do_pg_range: > > @@ -2047,7 +2043,6 @@ static void balloon_remove(struct hv_device *dev) > > struct hv_dynmem_device *dm = hv_get_drvdata(dev); > > struct hv_hotadd_state *has, *tmp; > > struct hv_hotadd_gap *gap, *tmp_gap; > > - unsigned long flags; > > > > if (dm->num_pages_ballooned != 0) > > pr_warn("Ballooned pages: %d\n", dm->num_pages_ballooned); > > @@ -2073,7 +2068,7 @@ static void balloon_remove(struct hv_device *dev) > > #endif > > } > > > > - spin_lock_irqsave(&dm_device.ha_lock, flags); > > + guard(spinlock_irqsave)(&dm_device.ha_lock); > > list_for_each_entry_safe(has, tmp, &dm->ha_region_list, list) { > > list_for_each_entry_safe(gap, tmp_gap, &has->gap_list, list) { > > list_del(&gap->list); > > @@ -2082,7 +2077,6 @@ static void balloon_remove(struct hv_device *dev) > > list_del(&has->list); > > kfree(has); > > } > > - spin_unlock_irqrestore(&dm_device.ha_lock, flags); > > } > > > > static int balloon_suspend(struct hv_device *hv_dev) > > > > --- > > base-commit: 3f01e9fed8454dcd89727016c3e5b2fbb8f8e50c > > change-id: 20230725-master-bbcd9205758b > > > > Best regards, > > -- > > Mitchell Levy <levymitchell0@xxxxxxxxx> > > These lines at the end of the patch look spurious. But Boqun has > already commented on that. > > Michael
