On Mon, 7 Oct 2024 at 07:24, Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> +static __always_inline __must_check bool file_ref_get(file_ref_t *ref)
> +{
> + /*
> + * Unconditionally increase the reference count with full
> + * ordering. The saturation and dead zones provide enough
> + * tolerance for this.
> + */
> + if (likely(!atomic_long_add_negative(1, &ref->refcnt)))
> + return true;
> +
> + /* Handle the cases inside the saturation and dead zones */
> + return __file_ref_get(ref);
> +}
Ack. This looks like it does the right thing.
That said, I wonder if we could clarify this code sequence by using
long old = atomic_long_fetch_inc(&ref->refcnt);
if (old > 0)
return true;
return __file_ref_get(ref, old);
instead, which would obviate all the games with using the magic
offset? IOW, it could use 0 as "free" instead of the special
off-by-one "-1 is free".
The reason we have that special "add_negative()" thing is that this
*used* to be much better on x86, because "xadd" was only added in the
i486, and used to be a few cycles slower than "lock ; add".
We got rid of i386 support some time ago, and the lack of xadd was
_one_ of the reasons for it (the supervisor mode WP bit handling issue
was the big one).
And yes, "add_negative()" still generates simpler code, in that it
just returns the sign flag, but I do feel it makes that code harder to
follow. And I say that despite being very aware of the whole
background to it all and somewhat used to this pattern.
Then file_ref_put() would do
old = atomic_long_fetch_dec(&ref->refcnt);
if (old > 1)
released = false;
else
released = __file_ref_put(ref, old);
(inside the preempt disable, of course), and again I feel like this
would be more legible.
And file_ref_read() would just become
unsigned long val = atomic_long_read(&ref->refcnt);
return val >= FILE_REF_RELEASED ? 0 : val;
without that odd "+1" to fix the off-by-one, and for the same reason
file_ref_init() would just become
atomic_long_set(&ref->refcnt, cnt);
with no off-by-one games.
I dunno. This is very much not a big deal, but I did react to the code
being a bit hard to read, and I think it could be friendlier to
humans..
Linus
