On Thu, Jun 25, 2026 at 09:22:01AM +0200, David Hildenbrand (Arm) wrote:
> On 6/24/26 16:57, Gregory Price wrote:
> > extern int offline_and_remove_memory(u64 start, u64 size);
> > +int offline_and_remove_memory_ranges(const struct range *ranges, int
> > nr_ranges);
> >
> > #else
> > static inline void try_offline_node(int nid) {}
> > @@ -283,6 +284,12 @@ static inline int remove_memory(u64 start, u64 size)
> > }
> >
> > static inline void __remove_memory(u64 start, u64 size) {}
> > +
> > +static inline int offline_and_remove_memory_ranges(const struct range
> > *ranges,
> > + int nr_ranges)
>
> Best to use "unsigned int" right from the start and use two tabs to indent.
>
ack, ack. need to reprogram my brain to two-indent style, i keep doing
this reflexively.
> > +int offline_and_remove_memory_ranges(const struct range *ranges, int
> > nr_ranges)
> > +{
> > + unsigned long mb_total = 0;
> > uint8_t *online_types, *tmp;
> > - int rc;
> > + int i, rc = 0;
> >
> > - if (!IS_ALIGNED(start, memory_block_size_bytes()) ||
> > - !IS_ALIGNED(size, memory_block_size_bytes()) || !size)
> > + if (!ranges || nr_ranges <= 0)
>
> With "unsigned int" this will be !nr_ranges.
>
> Wondering whether we would WARN_ON_ONCE() here.
>
Seems reasonable. Do we normally WARN when callers send dumb arguments?
Seems like sending -EINVAL is sufficient?
> > - online_types = kmalloc_array(mb_count, sizeof(*online_types),
> > + online_types = kmalloc_array(mb_total, sizeof(*online_types),
> > GFP_KERNEL);
>
> Is "mb_total" really more expressive than "mb_count"?
>
No, this was mostly my way ok keeping try of what was being moved around
while working it. I will change it back.
> > /*
> > - * In case we succeeded to offline all memory, remove it.
> > - * This cannot fail as it cannot get onlined in the meantime.
> > + * Phase 2: Remove each range. This essentially cannot fail as we hold
> > + * the hotplug lock . WARN if that assumption is ever broken.
> > */
> > if (!rc) {
> > - rc = try_remove_memory(start, size);
> > - if (rc)
> > - pr_err("%s: Failed to remove memory: %d", __func__, rc);
> > + for (i = 0; i < nr_ranges; i++) {
> > + rc = try_remove_memory(ranges[i].start,
> > + range_len(&ranges[i]));
> > + if (WARN_ON_ONCE(rc)) {
> > + pr_err("%s: Failed to remove memory: %d",
> > + __func__, rc);
> > + break;
>
> Do we really want to break? I'd say, just warn and continue, and fake rc == 0.
> Something is seriously messed up already, and we partially removed memory.
> There
> is no clean rollback possible.
>
> Similar to __remove_memory(), ignoring the error because it offlined it
> already.
>
This seems reasonable, will change to warn and continue + return error.
Sashiko actually pointed out there there's a corner condition here with
offline rollback, so i needed to tweak this chunk anyway.
~Gregory
