Re: [PATCH v3 1/3] resource: Use list_head to link resource sibling

[Baoquan He]

Hi Rob,

Thanks a lot for looking into this and involve Nico to this thread!

On 04/09/18 at 09:49am, Rob Herring wrote:
> +Nico who has been working on tinification of the kernel.
> 
> On Mon, Apr 9, 2018 at 4:08 AM, Baoquan He <bhe@xxxxxxxxxx> wrote:
> > The struct resource uses singly linked list to link siblings. It's not
> > easy to do reverse iteration on sibling list. So replace it with list_head.
> 
> Why is reverse iteration needed?

This is the explanation I made when Andrew helped to review the v1 post:
https://lkml.org/lkml/2018/3/23/78

Because we have been using kexec-tools utility to search available
System RAM space for loading kernel/initrd/purgatory from top to down.
That is done in user space by searching /proc/iomem. While later added
kexec_file interface, the searching code happened in kernel, and it
only search System RAM region bottom up, then take an area in that found
RAM region from top to down. We need unify these two interfaces on
behaviour since they are the same on essense from the users' point of
view, though implementation is different. As you know, the singly linked
list implementation of the current resource's sibling linking, makes the
searching from top to down very hard to satisfy people. 

Below is the v1 post, we make an temporary array to copy iomem_resource's
first level of children, then iterate the array reversedly. Andrew
suggested me to try list_head after reviewing. In fact we can optimize
that patch to only copy resource pointer into array, still the way is
ugly.
https://lkml.org/lkml/2018/3/21/952

Then Wei pasted a patch he had made as below. He didn't mention if he
also has requirement on reversed iteration of resource. That is an O(n*n)
way, from personal feelings, hard to say if it's bettern than v1 post.
https://lkml.org/lkml/2018/3/24/157

That's why I would like to have a try of the list_head linking.

> 
> > And code refactoring makes codes in kernel/resource.c more readable than
> > pointer operation.
> 
> resource_for_each_* helpers could solve that without the size increase.

Nico mentioned llist, that is a linux kernel standard singly linked
list, very elegant code existed. Still can not satisfy the reversed
iteration requirement.

> 
> > Besides, type of member variables of struct resource, sibling and child, are
> > changed from 'struct resource *' to 'struct list_head'. Kernel size will
> > increase because of those statically defined struct resource instances.
> 
> The DT struct device_node also has the same tree structure with
> parent, child, sibling pointers and converting to list_head had been
> on the todo list for a while. ACPI also has some tree walking
> functions (drivers/acpi/acpica/pstree.c). Perhaps there should be a
> common tree struct and helpers defined either on top of list_head or a
> new struct if that saves some size.

We have had many this kind of trees in kernel, the obvious examples is 
task_struct. And struct task_group {} too. They have used list_head
already.
struct task_struct {
	...
	/* Real parent process: */                                                                                 
        struct task_struct __rcu        *real_parent;                                                              
                                                                                                                   
        /* Recipient of SIGCHLD, wait4() reports: */                                                               
        struct task_struct __rcu        *parent;
	struct list_head                children;                                                                  
        struct list_head                sibling;
	...
}

		    root
                ///   |   \\\
               ///    |    \\\
              ///     |     \\\
             ///      |      \\\
          (child)<->(child)<->(child)
                         ///   |   \\\
                     ///       |      \\\
                   ///         |          \\\
                ///            |             \\\
             (grandchild)<->(grandchild)<->(grandchild)

If define an new struct, , like tree_list, or tlist?

struct tlist {
	void			*parent;
	struct list_head        sibling;                                                                                                         
	struct list_head        child;
}

Just a rough idea, feel it might not bring too much benefit compared
with direct list operation.

> 
> >
> > Signed-off-by: Baoquan He <bhe@xxxxxxxxxx>
> > ---
> > v2->v3:
> >   Make sibling() and first_child() global so that they can be called
> >   out of kernel/resource.c to simplify code.
> 
> These should probably be inline functions. Or exported if not.
> 
> >
> >   Fix several code bugs found by kbuild test robot.
> >
> >   Got report from lkp that kernel size increased. It's on purpose since
> >   the type change of sibling and child inside struct resource{}. For
> >   each struct resource variable, it will cost another 16 bytes on x86 64.
> 
> The size increase should be mentioned in the commit log. More
> generally, the size increase is 2 pointers.

Simply mentioned the size increase in this updated version. Yes, 2
pointers increased. Will add it to log ot make it more specifically.

Thanks
Baoquan

> >
> >  arch/sparc/kernel/ioport.c                  |   2 +-
> >  drivers/gpu/drm/drm_memory.c                |   3 +-
> >  drivers/gpu/drm/gma500/gtt.c                |   5 +-
> >  drivers/hv/vmbus_drv.c                      |  52 ++++----
> >  drivers/input/joystick/iforce/iforce-main.c |   4 +-
> >  drivers/nvdimm/e820.c                       |   2 +-
> >  drivers/nvdimm/namespace_devs.c             |   6 +-
> >  drivers/nvdimm/nd.h                         |   5 +-
> >  drivers/of/address.c                        |   4 +-
> >  drivers/parisc/lba_pci.c                    |   4 +-
> >  drivers/pci/host/vmd.c                      |   8 +-
> >  drivers/pci/probe.c                         |   2 +
> >  drivers/pci/setup-bus.c                     |   2 +-
> >  include/linux/ioport.h                      |   6 +-
> >  kernel/resource.c                           | 193 ++++++++++++++--------------
> >  15 files changed, 149 insertions(+), 149 deletions(-)
> >
> > diff --git a/arch/sparc/kernel/ioport.c b/arch/sparc/kernel/ioport.c
> > index 3bcef9ce74df..4e91fbbbedcc 100644
> > --- a/arch/sparc/kernel/ioport.c
> > +++ b/arch/sparc/kernel/ioport.c
> > @@ -669,7 +669,7 @@ static int sparc_io_proc_show(struct seq_file *m, void *v)
> >         struct resource *root = m->private, *r;
> >         const char *nm;
> >
> > -       for (r = root->child; r != NULL; r = r->sibling) {
> > +       list_for_each_entry(r, &root->child, sibling) {
> >                 if ((nm = r->name) == NULL) nm = "???";
> >                 seq_printf(m, "%016llx-%016llx: %s\n",
> >                                 (unsigned long long)r->start,
> > diff --git a/drivers/gpu/drm/drm_memory.c b/drivers/gpu/drm/drm_memory.c
> > index 3c54044214db..53e300a993dc 100644
> > --- a/drivers/gpu/drm/drm_memory.c
> > +++ b/drivers/gpu/drm/drm_memory.c
> > @@ -155,9 +155,8 @@ u64 drm_get_max_iomem(void)
> >         struct resource *tmp;
> >         resource_size_t max_iomem = 0;
> >
> > -       for (tmp = iomem_resource.child; tmp; tmp = tmp->sibling) {
> > +       list_for_each_entry(tmp, &iomem_resource.child, sibling)
> >                 max_iomem = max(max_iomem,  tmp->end);
> > -       }
> >
> >         return max_iomem;
> >  }
> > diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
> > index 3949b0990916..addd3bc009af 100644
> > --- a/drivers/gpu/drm/gma500/gtt.c
> > +++ b/drivers/gpu/drm/gma500/gtt.c
> > @@ -565,7 +565,7 @@ int psb_gtt_init(struct drm_device *dev, int resume)
> >  int psb_gtt_restore(struct drm_device *dev)
> >  {
> >         struct drm_psb_private *dev_priv = dev->dev_private;
> > -       struct resource *r = dev_priv->gtt_mem->child;
> > +       struct resource *r;
> >         struct gtt_range *range;
> >         unsigned int restored = 0, total = 0, size = 0;
> >
> > @@ -573,14 +573,13 @@ int psb_gtt_restore(struct drm_device *dev)
> >         mutex_lock(&dev_priv->gtt_mutex);
> >         psb_gtt_init(dev, 1);
> >
> > -       while (r != NULL) {
> > +       list_for_each_entry(r, &dev_priv->gtt_mem->child, sibling) {
> >                 range = container_of(r, struct gtt_range, resource);
> >                 if (range->pages) {
> >                         psb_gtt_insert(dev, range, 1);
> >                         size += range->resource.end - range->resource.start;
> >                         restored++;
> >                 }
> > -               r = r->sibling;
> >                 total++;
> >         }
> >         mutex_unlock(&dev_priv->gtt_mutex);
> > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > index bc65c4d79c1f..7ba8a25520d9 100644
> > --- a/drivers/hv/vmbus_drv.c
> > +++ b/drivers/hv/vmbus_drv.c
> > @@ -1413,9 +1413,8 @@ static acpi_status vmbus_walk_resources(struct acpi_resource *res, void *ctx)
> >  {
> >         resource_size_t start = 0;
> >         resource_size_t end = 0;
> > -       struct resource *new_res;
> > +       struct resource *new_res, *tmp;
> >         struct resource **old_res = &hyperv_mmio;
> > -       struct resource **prev_res = NULL;
> >
> >         switch (res->type) {
> >
> > @@ -1462,44 +1461,36 @@ static acpi_status vmbus_walk_resources(struct acpi_resource *res, void *ctx)
> >         /*
> >          * If two ranges are adjacent, merge them.
> >          */
> > -       do {
> > -               if (!*old_res) {
> > -                       *old_res = new_res;
> > -                       break;
> > -               }
> > -
> > -               if (((*old_res)->end + 1) == new_res->start) {
> > -                       (*old_res)->end = new_res->end;
> > +       if (!*old_res) {
> > +               *old_res = new_res;
> > +               return AE_OK;
> > +       }
> > +       tmp = *old_res;
> > +       list_for_each_entry_from(tmp, &tmp->parent->child, sibling) {
> > +               if ((tmp->end + 1) == new_res->start) {
> > +                       tmp->end = new_res->end;
> >                         kfree(new_res);
> >                         break;
> >                 }
> >
> > -               if ((*old_res)->start == new_res->end + 1) {
> > -                       (*old_res)->start = new_res->start;
> > +               if (tmp->start == new_res->end + 1) {
> > +                       tmp->start = new_res->start;
> >                         kfree(new_res);
> >                         break;
> >                 }
> >
> > -               if ((*old_res)->start > new_res->end) {
> > -                       new_res->sibling = *old_res;
> > -                       if (prev_res)
> > -                               (*prev_res)->sibling = new_res;
> > -                       *old_res = new_res;
> > +               if (tmp->start > new_res->end) {
> > +                       list_add(&new_res->sibling, tmp->sibling.prev);
> >                         break;
> >                 }
> > -
> > -               prev_res = old_res;
> > -               old_res = &(*old_res)->sibling;
> > -
> > -       } while (1);
> > +       }
> >
> >         return AE_OK;
> >  }
> >
> >  static int vmbus_acpi_remove(struct acpi_device *device)
> >  {
> > -       struct resource *cur_res;
> > -       struct resource *next_res;
> > +       struct resource *res;
> >
> >         if (hyperv_mmio) {
> >                 if (fb_mmio) {
> > @@ -1508,10 +1499,9 @@ static int vmbus_acpi_remove(struct acpi_device *device)
> >                         fb_mmio = NULL;
> >                 }
> >
> > -               for (cur_res = hyperv_mmio; cur_res; cur_res = next_res) {
> > -                       next_res = cur_res->sibling;
> > -                       kfree(cur_res);
> > -               }
> > +               res = hyperv_mmio;
> > +               list_for_each_entry_from(res, &res->parent->child, sibling)
> > +                       kfree(res);
> >         }
> >
> >         return 0;
> > @@ -1597,7 +1587,8 @@ int vmbus_allocate_mmio(struct resource **new, struct hv_device *device_obj,
> >                 }
> >         }
> >
> > -       for (iter = hyperv_mmio; iter; iter = iter->sibling) {
> > +       iter = hyperv_mmio;
> > +       list_for_each_entry_from(iter, &iter->parent->child, sibling) {
> >                 if ((iter->start >= max) || (iter->end <= min))
> >                         continue;
> >
> > @@ -1640,7 +1631,8 @@ void vmbus_free_mmio(resource_size_t start, resource_size_t size)
> >         struct resource *iter;
> >
> >         down(&hyperv_mmio_lock);
> > -       for (iter = hyperv_mmio; iter; iter = iter->sibling) {
> > +       iter = hyperv_mmio;
> > +       list_for_each_entry_from(iter, &iter->parent->child, sibling) {
> >                 if ((iter->start >= start + size) || (iter->end <= start))
> >                         continue;
> >
> > diff --git a/drivers/input/joystick/iforce/iforce-main.c b/drivers/input/joystick/iforce/iforce-main.c
> > index daeeb4c7e3b0..5c0be27b33ff 100644
> > --- a/drivers/input/joystick/iforce/iforce-main.c
> > +++ b/drivers/input/joystick/iforce/iforce-main.c
> > @@ -305,8 +305,8 @@ int iforce_init_device(struct iforce *iforce)
> >         iforce->device_memory.end = 200;
> >         iforce->device_memory.flags = IORESOURCE_MEM;
> >         iforce->device_memory.parent = NULL;
> > -       iforce->device_memory.child = NULL;
> > -       iforce->device_memory.sibling = NULL;
> > +       INIT_LIST_HEAD(&iforce->device_memory.child);
> > +       INIT_LIST_HEAD(&iforce->device_memory.sibling);
> >
> >  /*
> >   * Wait until device ready - until it sends its first response.
> > diff --git a/drivers/nvdimm/e820.c b/drivers/nvdimm/e820.c
> > index 6f9a6ffd7cde..513e661bb0d8 100644
> > --- a/drivers/nvdimm/e820.c
> > +++ b/drivers/nvdimm/e820.c
> > @@ -53,7 +53,7 @@ static int e820_pmem_probe(struct platform_device *pdev)
> >                 goto err;
> >         platform_set_drvdata(pdev, nvdimm_bus);
> >
> > -       for (p = iomem_resource.child; p ; p = p->sibling) {
> > +       list_for_each_entry(p, &iomem_resource.child, sibling) {
> >                 struct nd_region_desc ndr_desc;
> >
> >                 if (p->desc != IORES_DESC_PERSISTENT_MEMORY_LEGACY)
> > diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
> > index 658ada497be0..bcbdf5335909 100644
> > --- a/drivers/nvdimm/namespace_devs.c
> > +++ b/drivers/nvdimm/namespace_devs.c
> > @@ -637,7 +637,7 @@ static resource_size_t scan_allocate(struct nd_region *nd_region,
> >   retry:
> >         first = 0;
> >         for_each_dpa_resource(ndd, res) {
> > -               struct resource *next = res->sibling, *new_res = NULL;
> > +               struct resource *next = sibling(res), *new_res = NULL;
> >                 resource_size_t allocate, available = 0;
> >                 enum alloc_loc loc = ALLOC_ERR;
> >                 const char *action;
> > @@ -763,7 +763,7 @@ static resource_size_t scan_allocate(struct nd_region *nd_region,
> >          * an initial "pmem-reserve pass".  Only do an initial BLK allocation
> >          * when none of the DPA space is reserved.
> >          */
> > -       if ((is_pmem || !ndd->dpa.child) && n == to_allocate)
> > +       if ((is_pmem || list_empty(&ndd->dpa.child)) && n == to_allocate)
> >                 return init_dpa_allocation(label_id, nd_region, nd_mapping, n);
> >         return n;
> >  }
> > @@ -779,7 +779,7 @@ static int merge_dpa(struct nd_region *nd_region,
> >   retry:
> >         for_each_dpa_resource(ndd, res) {
> >                 int rc;
> > -               struct resource *next = res->sibling;
> > +               struct resource *next = sibling(res);
> >                 resource_size_t end = res->start + resource_size(res);
> >
> >                 if (!next || strcmp(res->name, label_id->id) != 0
> > diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
> > index 184e070d50a2..1dc0b2370bd1 100644
> > --- a/drivers/nvdimm/nd.h
> > +++ b/drivers/nvdimm/nd.h
> > @@ -102,11 +102,10 @@ unsigned sizeof_namespace_label(struct nvdimm_drvdata *ndd);
> >                 (unsigned long long) (res ? res->start : 0), ##arg)
> >
> >  #define for_each_dpa_resource(ndd, res) \
> > -       for (res = (ndd)->dpa.child; res; res = res->sibling)
> > +       list_for_each_entry(res, &(ndd)->dpa.child, sibling)
> >
> >  #define for_each_dpa_resource_safe(ndd, res, next) \
> > -       for (res = (ndd)->dpa.child, next = res ? res->sibling : NULL; \
> > -                       res; res = next, next = next ? next->sibling : NULL)
> > +       list_for_each_entry_safe(res, next, &(ndd)->dpa.child, sibling)
> >
> >  struct nd_percpu_lane {
> >         int count;
> > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > index 53349912ac75..e2e25719ab52 100644
> > --- a/drivers/of/address.c
> > +++ b/drivers/of/address.c
> > @@ -330,7 +330,9 @@ int of_pci_range_to_resource(struct of_pci_range *range,
> >  {
> >         int err;
> >         res->flags = range->flags;
> > -       res->parent = res->child = res->sibling = NULL;
> > +       res->parent = NULL;
> > +       INIT_LIST_HEAD(&res->child);
> > +       INIT_LIST_HEAD(&res->sibling);
> >         res->name = np->full_name;
> >
> >         if (res->flags & IORESOURCE_IO) {
> > diff --git a/drivers/parisc/lba_pci.c b/drivers/parisc/lba_pci.c
> > index 69bd98421eb1..84f04418ae0b 100644
> > --- a/drivers/parisc/lba_pci.c
> > +++ b/drivers/parisc/lba_pci.c
> > @@ -170,8 +170,8 @@ lba_dump_res(struct resource *r, int d)
> >         for (i = d; i ; --i) printk(" ");
> >         printk(KERN_DEBUG "%p [%lx,%lx]/%lx\n", r,
> >                 (long)r->start, (long)r->end, r->flags);
> > -       lba_dump_res(r->child, d+2);
> > -       lba_dump_res(r->sibling, d);
> > +       lba_dump_res(first_child(&r->child), d+2);
> > +       lba_dump_res(sibling(r), d);
> >  }
> >
> >
> > diff --git a/drivers/pci/host/vmd.c b/drivers/pci/host/vmd.c
> > index 930a8fa08bd6..c3000af903ea 100644
> > --- a/drivers/pci/host/vmd.c
> > +++ b/drivers/pci/host/vmd.c
> > @@ -520,14 +520,14 @@ static struct pci_ops vmd_ops = {
> >
> >  static void vmd_attach_resources(struct vmd_dev *vmd)
> >  {
> > -       vmd->dev->resource[VMD_MEMBAR1].child = &vmd->resources[1];
> > -       vmd->dev->resource[VMD_MEMBAR2].child = &vmd->resources[2];
> > +       list_add(&vmd->resources[1].sibling, &vmd->dev->resource[VMD_MEMBAR1].child);
> > +       list_add(&vmd->resources[2].sibling, &vmd->dev->resource[VMD_MEMBAR2].child);
> >  }
> >
> >  static void vmd_detach_resources(struct vmd_dev *vmd)
> >  {
> > -       vmd->dev->resource[VMD_MEMBAR1].child = NULL;
> > -       vmd->dev->resource[VMD_MEMBAR2].child = NULL;
> > +       INIT_LIST_HEAD(&vmd->dev->resource[VMD_MEMBAR1].child);
> > +       INIT_LIST_HEAD(&vmd->dev->resource[VMD_MEMBAR2].child);
> >  }
> >
> >  /*
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index ac91b6fd0bcd..d162c77bec29 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -59,6 +59,8 @@ static struct resource *get_pci_domain_busn_res(int domain_nr)
> >         r->res.start = 0;
> >         r->res.end = 0xff;
> >         r->res.flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED;
> > +       INIT_LIST_HEAD(&r->res.child);
> > +       INIT_LIST_HEAD(&r->res.sibling);
> >
> >         list_add_tail(&r->list, &pci_domain_busn_res_list);
> >
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index 072784f55ea5..0d5e30004ca6 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -2107,7 +2107,7 @@ int pci_reassign_bridge_resources(struct pci_dev *bridge, unsigned long type)
> >                                 continue;
> >
> >                         /* Ignore BARs which are still in use */
> > -                       if (res->child)
> > +                       if (!list_empty(&res->child))
> >                                 continue;
> >
> >                         ret = add_to_list(&saved, bridge, res, 0, 0);
> > diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> > index da0ebaec25f0..03d1510f03e0 100644
> > --- a/include/linux/ioport.h
> > +++ b/include/linux/ioport.h
> > @@ -22,7 +22,8 @@ struct resource {
> >         const char *name;
> >         unsigned long flags;
> >         unsigned long desc;
> > -       struct resource *parent, *sibling, *child;
> > +       struct list_head child, sibling;
> > +       struct resource *parent;
> >  };
> >
> >  /*
> > @@ -189,6 +190,8 @@ extern int allocate_resource(struct resource *root, struct resource *new,
> >                                                        resource_size_t,
> >                                                        resource_size_t),
> >                              void *alignf_data);
> > +extern struct resource *sibling(struct resource *res);
> > +extern struct resource *first_child(struct list_head *head);
> >  struct resource *lookup_resource(struct resource *root, resource_size_t start);
> >  int adjust_resource(struct resource *res, resource_size_t start,
> >                     resource_size_t size);
> > @@ -215,7 +218,6 @@ static inline bool resource_contains(struct resource *r1, struct resource *r2)
> >         return r1->start <= r2->start && r1->end >= r2->end;
> >  }
> >
> > -
> >  /* Convenience shorthand with allocation */
> >  #define request_region(start,n,name)           __request_region(&ioport_resource, (start), (n), (name), 0)
> >  #define request_muxed_region(start,n,name)     __request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
> > diff --git a/kernel/resource.c b/kernel/resource.c
> > index e270b5048988..473c624606f9 100644
> > --- a/kernel/resource.c
> > +++ b/kernel/resource.c
> > @@ -31,6 +31,8 @@ struct resource ioport_resource = {
> >         .start  = 0,
> >         .end    = IO_SPACE_LIMIT,
> >         .flags  = IORESOURCE_IO,
> > +       .sibling = LIST_HEAD_INIT(ioport_resource.sibling),
> > +       .child  = LIST_HEAD_INIT(ioport_resource.child),
> >  };
> >  EXPORT_SYMBOL(ioport_resource);
> >
> > @@ -39,6 +41,8 @@ struct resource iomem_resource = {
> >         .start  = 0,
> >         .end    = -1,
> >         .flags  = IORESOURCE_MEM,
> > +       .sibling = LIST_HEAD_INIT(iomem_resource.sibling),
> > +       .child  = LIST_HEAD_INIT(iomem_resource.child),
> >  };
> >  EXPORT_SYMBOL(iomem_resource);
> >
> > @@ -57,20 +61,32 @@ static DEFINE_RWLOCK(resource_lock);
> >   * by boot mem after the system is up. So for reusing the resource entry
> >   * we need to remember the resource.
> >   */
> > -static struct resource *bootmem_resource_free;
> > +static struct list_head bootmem_resource_free = LIST_HEAD_INIT(bootmem_resource_free);
> >  static DEFINE_SPINLOCK(bootmem_resource_lock);
> >
> > +struct resource *sibling(struct resource *res)
> > +{
> > +       if (res->parent && !list_is_last(&res->sibling, &res->parent->child))
> > +               return list_next_entry(res, sibling);
> > +       return NULL;
> > +}
> > +
> > +struct resource *first_child(struct list_head *head)
> > +{
> > +       return list_first_entry_or_null(head, struct resource, sibling);
> > +}
> > +
> >  static struct resource *next_resource(struct resource *p, bool sibling_only)
> >  {
> >         /* Caller wants to traverse through siblings only */
> >         if (sibling_only)
> > -               return p->sibling;
> > +               return sibling(p);
> >
> > -       if (p->child)
> > -               return p->child;
> > -       while (!p->sibling && p->parent)
> > +       if (!list_empty(&p->child))
> > +               return first_child(&p->child);
> > +       while (!sibling(p) && p->parent)
> >                 p = p->parent;
> > -       return p->sibling;
> > +       return sibling(p);
> >  }
> >
> >  static void *r_next(struct seq_file *m, void *v, loff_t *pos)
> > @@ -90,7 +106,7 @@ static void *r_start(struct seq_file *m, loff_t *pos)
> >         struct resource *p = m->private;
> >         loff_t l = 0;
> >         read_lock(&resource_lock);
> > -       for (p = p->child; p && l < *pos; p = r_next(m, p, &l))
> > +       for (p = first_child(&p->child); p && l < *pos; p = r_next(m, p, &l))
> >                 ;
> >         return p;
> >  }
> > @@ -186,8 +202,7 @@ static void free_resource(struct resource *res)
> >
> >         if (!PageSlab(virt_to_head_page(res))) {
> >                 spin_lock(&bootmem_resource_lock);
> > -               res->sibling = bootmem_resource_free;
> > -               bootmem_resource_free = res;
> > +               list_add(&res->sibling, &bootmem_resource_free);
> >                 spin_unlock(&bootmem_resource_lock);
> >         } else {
> >                 kfree(res);
> > @@ -199,10 +214,9 @@ static struct resource *alloc_resource(gfp_t flags)
> >         struct resource *res = NULL;
> >
> >         spin_lock(&bootmem_resource_lock);
> > -       if (bootmem_resource_free) {
> > -               res = bootmem_resource_free;
> > -               bootmem_resource_free = res->sibling;
> > -       }
> > +       res = first_child(&bootmem_resource_free);
> > +       if (res)
> > +               list_del(&res->sibling);
> >         spin_unlock(&bootmem_resource_lock);
> >
> >         if (res)
> > @@ -210,6 +224,8 @@ static struct resource *alloc_resource(gfp_t flags)
> >         else
> >                 res = kzalloc(sizeof(struct resource), flags);
> >
> > +       INIT_LIST_HEAD(&res->child);
> > +       INIT_LIST_HEAD(&res->sibling);
> >         return res;
> >  }
> >
> > @@ -218,7 +234,7 @@ static struct resource * __request_resource(struct resource *root, struct resour
> >  {
> >         resource_size_t start = new->start;
> >         resource_size_t end = new->end;
> > -       struct resource *tmp, **p;
> > +       struct resource *tmp;
> >
> >         if (end < start)
> >                 return root;
> > @@ -226,64 +242,62 @@ static struct resource * __request_resource(struct resource *root, struct resour
> >                 return root;
> >         if (end > root->end)
> >                 return root;
> > -       p = &root->child;
> > -       for (;;) {
> > -               tmp = *p;
> > -               if (!tmp || tmp->start > end) {
> > -                       new->sibling = tmp;
> > -                       *p = new;
> > +
> > +       if (list_empty(&root->child)) {
> > +               list_add(&new->sibling, &root->child);
> > +               new->parent = root;
> > +               INIT_LIST_HEAD(&new->child);
> > +               return NULL;
> > +       }
> > +
> > +       list_for_each_entry(tmp, &root->child, sibling) {
> > +               if (tmp->start > end) {
> > +                       list_add(&new->sibling, tmp->sibling.prev);
> >                         new->parent = root;
> > +                       INIT_LIST_HEAD(&new->child);
> >                         return NULL;
> >                 }
> > -               p = &tmp->sibling;
> >                 if (tmp->end < start)
> >                         continue;
> >                 return tmp;
> >         }
> > +
> > +       list_add_tail(&new->sibling, &root->child);
> > +       new->parent = root;
> > +       INIT_LIST_HEAD(&new->child);
> > +       return NULL;
> >  }
> >
> >  static int __release_resource(struct resource *old, bool release_child)
> >  {
> > -       struct resource *tmp, **p, *chd;
> > +       struct resource *tmp, *next, *chd;
> >
> > -       p = &old->parent->child;
> > -       for (;;) {
> > -               tmp = *p;
> > -               if (!tmp)
> > -                       break;
> > +       list_for_each_entry_safe(tmp, next, &old->parent->child, sibling) {
> >                 if (tmp == old) {
> > -                       if (release_child || !(tmp->child)) {
> > -                               *p = tmp->sibling;
> > +                       if (release_child || list_empty(&tmp->child)) {
> > +                               list_del(&tmp->sibling);
> >                         } else {
> > -                               for (chd = tmp->child;; chd = chd->sibling) {
> > +                               list_for_each_entry(chd, &tmp->child, sibling)
> >                                         chd->parent = tmp->parent;
> > -                                       if (!(chd->sibling))
> > -                                               break;
> > -                               }
> > -                               *p = tmp->child;
> > -                               chd->sibling = tmp->sibling;
> > +                               list_splice(&tmp->child, tmp->sibling.prev);
> > +                               list_del(&tmp->sibling);
> >                         }
> > +
> >                         old->parent = NULL;
> >                         return 0;
> >                 }
> > -               p = &tmp->sibling;
> >         }
> >         return -EINVAL;
> >  }
> >
> >  static void __release_child_resources(struct resource *r)
> >  {
> > -       struct resource *tmp, *p;
> > +       struct resource *tmp, *next;
> >         resource_size_t size;
> >
> > -       p = r->child;
> > -       r->child = NULL;
> > -       while (p) {
> > -               tmp = p;
> > -               p = p->sibling;
> > -
> > +       list_for_each_entry_safe(tmp, next, &r->child, sibling) {
> >                 tmp->parent = NULL;
> > -               tmp->sibling = NULL;
> > +               INIT_LIST_HEAD(&tmp->sibling);
> >                 __release_child_resources(tmp);
> >
> >                 printk(KERN_DEBUG "release child resource %pR\n", tmp);
> > @@ -292,6 +306,8 @@ static void __release_child_resources(struct resource *r)
> >                 tmp->start = 0;
> >                 tmp->end = size - 1;
> >         }
> > +
> > +       INIT_LIST_HEAD(&tmp->child);
> >  }
> >
> >  void release_child_resources(struct resource *r)
> > @@ -376,7 +392,8 @@ static int find_next_iomem_res(struct resource *res, unsigned long desc,
> >
> >         read_lock(&resource_lock);
> >
> > -       for (p = iomem_resource.child; p; p = next_resource(p, sibling_only)) {
> > +       for (p = first_child(&iomem_resource.child); p;
> > +                       p = next_resource(p, sibling_only)) {
> >                 if ((p->flags & res->flags) != res->flags)
> >                         continue;
> >                 if ((desc != IORES_DESC_NONE) && (desc != p->desc))
> > @@ -564,7 +581,7 @@ int region_intersects(resource_size_t start, size_t size, unsigned long flags,
> >         struct resource *p;
> >
> >         read_lock(&resource_lock);
> > -       for (p = iomem_resource.child; p ; p = p->sibling) {
> > +       list_for_each_entry(p, &iomem_resource.child, sibling) {
> >                 bool is_type = (((p->flags & flags) == flags) &&
> >                                 ((desc == IORES_DESC_NONE) ||
> >                                  (desc == p->desc)));
> > @@ -618,7 +635,7 @@ static int __find_resource(struct resource *root, struct resource *old,
> >                          resource_size_t  size,
> >                          struct resource_constraint *constraint)
> >  {
> > -       struct resource *this = root->child;
> > +       struct resource *this = first_child(&root->child);
> >         struct resource tmp = *new, avail, alloc;
> >
> >         tmp.start = root->start;
> > @@ -628,7 +645,7 @@ static int __find_resource(struct resource *root, struct resource *old,
> >          */
> >         if (this && this->start == root->start) {
> >                 tmp.start = (this == old) ? old->start : this->end + 1;
> > -               this = this->sibling;
> > +               this = sibling(this);
> >         }
> >         for(;;) {
> >                 if (this)
> > @@ -663,7 +680,7 @@ next:               if (!this || this->end == root->end)
> >
> >                 if (this != old)
> >                         tmp.start = this->end + 1;
> > -               this = this->sibling;
> > +               this = sibling(this);
> >         }
> >         return -EBUSY;
> >  }
> > @@ -707,7 +724,7 @@ static int reallocate_resource(struct resource *root, struct resource *old,
> >                 goto out;
> >         }
> >
> > -       if (old->child) {
> > +       if (!list_empty(&old->child)) {
> >                 err = -EBUSY;
> >                 goto out;
> >         }
> > @@ -788,7 +805,7 @@ struct resource *lookup_resource(struct resource *root, resource_size_t start)
> >         struct resource *res;
> >
> >         read_lock(&resource_lock);
> > -       for (res = root->child; res; res = res->sibling) {
> > +       list_for_each_entry(res, &root->child, sibling) {
> >                 if (res->start == start)
> >                         break;
> >         }
> > @@ -821,32 +838,27 @@ static struct resource * __insert_resource(struct resource *parent, struct resou
> >                         break;
> >         }
> >
> > -       for (next = first; ; next = next->sibling) {
> > +       for (next = first; ; next = sibling(next)) {
> >                 /* Partial overlap? Bad, and unfixable */
> >                 if (next->start < new->start || next->end > new->end)
> >                         return next;
> > -               if (!next->sibling)
> > +               if (!sibling(next))
> >                         break;
> > -               if (next->sibling->start > new->end)
> > +               if (sibling(next)->start > new->end)
> >                         break;
> >         }
> > -
> >         new->parent = parent;
> > -       new->sibling = next->sibling;
> > -       new->child = first;
> > +       list_add(&new->sibling, &next->sibling);
> > +       INIT_LIST_HEAD(&new->child);
> >
> > -       next->sibling = NULL;
> > -       for (next = first; next; next = next->sibling)
> > +       /*
> > +        * From first to next, they all fall into new's region, so change them
> > +        * as new's children.
> > +        */
> > +       list_cut_position(&new->child, first->sibling.prev, &next->sibling);
> > +       list_for_each_entry(next, &new->child, sibling)
> >                 next->parent = new;
> >
> > -       if (parent->child == first) {
> > -               parent->child = new;
> > -       } else {
> > -               next = parent->child;
> > -               while (next->sibling != first)
> > -                       next = next->sibling;
> > -               next->sibling = new;
> > -       }
> >         return NULL;
> >  }
> >
> > @@ -968,19 +980,17 @@ static int __adjust_resource(struct resource *res, resource_size_t start,
> >         if ((start < parent->start) || (end > parent->end))
> >                 goto out;
> >
> > -       if (res->sibling && (res->sibling->start <= end))
> > +       if (sibling(res) && (sibling(res)->start <= end))
> >                 goto out;
> >
> > -       tmp = parent->child;
> > -       if (tmp != res) {
> > -               while (tmp->sibling != res)
> > -                       tmp = tmp->sibling;
> > +       if (res->sibling.prev != &parent->child) {
> > +               tmp = list_prev_entry(res, sibling);
> >                 if (start <= tmp->end)
> >                         goto out;
> >         }
> >
> >  skip:
> > -       for (tmp = res->child; tmp; tmp = tmp->sibling)
> > +       list_for_each_entry(tmp, &res->child, sibling)
> >                 if ((tmp->start < start) || (tmp->end > end))
> >                         goto out;
> >
> > @@ -1205,34 +1215,32 @@ EXPORT_SYMBOL(__request_region);
> >  void __release_region(struct resource *parent, resource_size_t start,
> >                         resource_size_t n)
> >  {
> > -       struct resource **p;
> > +       struct resource *res;
> >         resource_size_t end;
> >
> > -       p = &parent->child;
> > +       res = first_child(&parent->child);
> >         end = start + n - 1;
> >
> >         write_lock(&resource_lock);
> >
> >         for (;;) {
> > -               struct resource *res = *p;
> > -
> >                 if (!res)
> >                         break;
> >                 if (res->start <= start && res->end >= end) {
> >                         if (!(res->flags & IORESOURCE_BUSY)) {
> > -                               p = &res->child;
> > +                               res = first_child(&res->child);
> >                                 continue;
> >                         }
> >                         if (res->start != start || res->end != end)
> >                                 break;
> > -                       *p = res->sibling;
> > +                       list_del(&res->sibling);
> >                         write_unlock(&resource_lock);
> >                         if (res->flags & IORESOURCE_MUXED)
> >                                 wake_up(&muxed_resource_wait);
> >                         free_resource(res);
> >                         return;
> >                 }
> > -               p = &res->sibling;
> > +               res = sibling(res);
> >         }
> >
> >         write_unlock(&resource_lock);
> > @@ -1267,9 +1275,7 @@ EXPORT_SYMBOL(__release_region);
> >  int release_mem_region_adjustable(struct resource *parent,
> >                         resource_size_t start, resource_size_t size)
> >  {
> > -       struct resource **p;
> > -       struct resource *res;
> > -       struct resource *new_res;
> > +       struct resource *res, *new_res;
> >         resource_size_t end;
> >         int ret = -EINVAL;
> >
> > @@ -1280,16 +1286,16 @@ int release_mem_region_adjustable(struct resource *parent,
> >         /* The alloc_resource() result gets checked later */
> >         new_res = alloc_resource(GFP_KERNEL);
> >
> > -       p = &parent->child;
> > +       res = first_child(&parent->child);
> >         write_lock(&resource_lock);
> >
> > -       while ((res = *p)) {
> > +       while ((res)) {
> >                 if (res->start >= end)
> >                         break;
> >
> >                 /* look for the next resource if it does not fit into */
> >                 if (res->start > start || res->end < end) {
> > -                       p = &res->sibling;
> > +                       res = sibling(res);
> >                         continue;
> >                 }
> >
> > @@ -1297,14 +1303,14 @@ int release_mem_region_adjustable(struct resource *parent,
> >                         break;
> >
> >                 if (!(res->flags & IORESOURCE_BUSY)) {
> > -                       p = &res->child;
> > +                       res = first_child(&res->child);
> >                         continue;
> >                 }
> >
> >                 /* found the target resource; let's adjust accordingly */
> >                 if (res->start == start && res->end == end) {
> >                         /* free the whole entry */
> > -                       *p = res->sibling;
> > +                       list_del(&res->sibling);
> >                         free_resource(res);
> >                         ret = 0;
> >                 } else if (res->start == start && res->end != end) {
> > @@ -1327,14 +1333,13 @@ int release_mem_region_adjustable(struct resource *parent,
> >                         new_res->flags = res->flags;
> >                         new_res->desc = res->desc;
> >                         new_res->parent = res->parent;
> > -                       new_res->sibling = res->sibling;
> > -                       new_res->child = NULL;
> > +                       INIT_LIST_HEAD(&new_res->child);
> >
> >                         ret = __adjust_resource(res, res->start,
> >                                                 start - res->start);
> >                         if (ret)
> >                                 break;
> > -                       res->sibling = new_res;
> > +                       list_add(&new_res->sibling, &res->sibling);
> >                         new_res = NULL;
> >                 }
> >
> > @@ -1515,7 +1520,7 @@ static int __init reserve_setup(char *str)
> >                         res->end = io_start + io_num - 1;
> >                         res->flags |= IORESOURCE_BUSY;
> >                         res->desc = IORES_DESC_NONE;
> > -                       res->child = NULL;
> > +                       INIT_LIST_HEAD(&res->child);
> >                         if (request_resource(parent, res) == 0)
> >                                 reserved = x+1;
> >                 }
> > @@ -1535,7 +1540,7 @@ int iomem_map_sanity_check(resource_size_t addr, unsigned long size)
> >         loff_t l;
> >
> >         read_lock(&resource_lock);
> > -       for (p = p->child; p ; p = r_next(NULL, p, &l)) {
> > +       for (p = first_child(&p->child); p; p = r_next(NULL, p, &l)) {
> >                 /*
> >                  * We can probably skip the resources without
> >                  * IORESOURCE_IO attribute?
> > @@ -1591,7 +1596,7 @@ bool iomem_is_exclusive(u64 addr)
> >         addr = addr & PAGE_MASK;
> >
> >         read_lock(&resource_lock);
> > -       for (p = p->child; p ; p = r_next(NULL, p, &l)) {
> > +       for (p = first_child(&p->child); p; p = r_next(NULL, p, &l)) {
> >                 /*
> >                  * We can probably skip the resources without
> >                  * IORESOURCE_IO attribute?
> > --
> > 2.13.6
> >