Hi Hannes,
On Wed, 2026-05-27 at 11:28 +0200, Hannes Reinecke wrote:
> On 5/22/26 18:44, Martin Wilck wrote:
> > Add a set of simple functions to handle refcounted pointers.
> >
> > Signed-off-by: Martin Wilck <[email protected]>
> > Reviewed-by: Benjamin Marzinski <[email protected]>
> > ---
> > libmpathutil/libmpathutil.version | 7 +++++
> > libmpathutil/util.c | 45
> > ++++++++++++++++++++++++++++---
> > libmpathutil/util.h | 5 ++++
> > 3 files changed, 53 insertions(+), 4 deletions(-)
> >
> That really looks like the Hazard pointers from Paul McK and Mathieu
> Desnoyers. Maybe have a look and see if you cannot re-use the
> approach here.
Thanks for the hint. Hazard pointers are a far more advanced
complicated concept, focussed on cache efficiency and performance.
That's important for the kernel, but it would be over-engineered for
libmultipath, IMO. We just need to make sure that the pointer is not
freed as long as it's still referenced somewhere.
AFAICS, Mathieu's code is targeted at the kernel. We could consider
using some generic user-space hazard pointer implementation for
libmultipath if there is something we could take off the shelf (like,
as a part of liburcu, which we're using anyway). But I don't see a
strong need for it. There's no doubt that libmultipath is inefficient
in many ways, but it isn't CPU-intensive, and thus correctness and
simplicity matter more than efficiency.
>
> > diff --git a/libmpathutil/libmpathutil.version
> > b/libmpathutil/libmpathutil.version
> > index c69120d..51594ad 100644
> > --- a/libmpathutil/libmpathutil.version
> > +++ b/libmpathutil/libmpathutil.version
> > @@ -194,3 +194,10 @@ global:
> > local:
> > *;
> > };
> > +
> > +LIBMPATHUTIL_6.1 {
> > +global:
> > + alloc_shared_ptr;
> > + get_shared_ptr;
> > + put_shared_ptr;
> > +} LIBMPATHUTIL_6.0;
> > diff --git a/libmpathutil/util.c b/libmpathutil/util.c
> > index 23a9797..3c623ec 100644
> > --- a/libmpathutil/util.c
> > +++ b/libmpathutil/util.c
> > @@ -12,15 +12,13 @@
> > #include <dirent.h>
> > #include <unistd.h>
> > #include <errno.h>
> > +#include <urcu/uatomic.h>
> > #include "mt-udev-wrap.h"
> >
> > #include "util.h"
> > #include "debug.h"
> > -#include "checkers.h"
> > #include "vector.h"
> > -#include "structs.h"
> > -#include "config.h"
> > -#include "log.h"
> > +#include "list.h"
> >
> > size_t
> > strchop(char *str)
> > @@ -384,3 +382,42 @@ void cleanup_udev_device(struct udev_device
> > **udd)
> > if (*udd)
> > udev_device_unref(*udd);
> > }
> > +
> > +struct shared_ptr {
> > + long refcnt;
> > + void (*destructor)(void *);
> > + char __attribute__((aligned(sizeof(void *)))) ptr[];
> > +};
> > +
> > +void *alloc_shared_ptr(size_t size, void (*destructor)(void *))
> > +{
> > + struct shared_ptr *sp = malloc(sizeof(*sp) + size);
> > +
> > + if (!sp)
> > + return NULL;
> > + uatomic_set(&sp->refcnt, 1);
> > + sp->destructor = destructor;
> > + return sp->ptr;
> > +}
> > +
> > +void get_shared_ptr(void *ptr)
> > +{
> > + struct shared_ptr *sp = container_of(ptr, struct
> > shared_ptr, ptr);
> > +
> > + if (uatomic_add_return(&sp->refcnt, 1) < 0)
> > + condlog(0, "%s: refcount overflow", __func__);
> > +}
> > +
> > +void put_shared_ptr(void *ptr)
> > +{
> > + struct shared_ptr *sp;
> > +
> > + if (!ptr)
> > + return;
> > + sp = container_of(ptr, struct shared_ptr, ptr);
> > + if (uatomic_sub_return(&sp->refcnt, 1) == 0) {
> > + if (sp->destructor)
> > + sp->destructor(ptr);
> > + free(sp);
> > + }
> > +}
>
> I _think_ the could be implemented better using RCU primitives;
> in the end, what you really care about is whether the pointer is
> valid (9which is what RCU provides).
What we want to achieve is just that (the destructor is called and) the
memory is freed when the last reference is dropped, but no sooner. It's
the same concept that the kernel uses for refcounted objects all over
the place.
> Or indeed use the hazard pointer approach, and count the number
> of RCU references to that object ...
Like I said, I'd like to keep it simple. The basic refcounting
that this patch implements does the job for libmultipath.
I just created this admittedly simple abstraction because the multipath
code was using reference counters in multiple places, and I wanted to
consolidate the code and add a unit test for it. It's not the purpose
of this patch set to optimize for performance. If we want to optimize
it, the API is simple enough that we can substitute the implementation
easily with something more efficient in the future.
>
> > diff --git a/libmpathutil/util.h b/libmpathutil/util.h
> > index aed1bc1..ffbb182 100644
> > --- a/libmpathutil/util.h
> > +++ b/libmpathutil/util.h
> > @@ -160,4 +160,9 @@ void cleanup_charp(char **p);
> > void cleanup_ucharp(unsigned char **p);
> > void cleanup_udev_device(struct udev_device **udd);
> > void cleanup_bitfield(union bitfield **p);
> > +
> > +void *alloc_shared_ptr(size_t size, void (*destructor)(void *));
> > +void get_shared_ptr(void *ptr);
> > +void put_shared_ptr(void *ptr);
> > +
> > #endif /* UTIL_H_INCLUDED */
>
> ... or just leave it as it is if the approach is to complex.
> I doubt it's time-critical.
>
Exactly. I'd like to see what Ben and others think.
Regards,
Martin
> Cheers,
>
> Hannes
