Re: git: e6bc24b0385a - main - kref: switch internal type to atomic_t and bring back const to kref_read

On 12/14/22, Alexander Richardson <arichardson@freebsd.org> wrote:
> On Tue, 13 Dec 2022 at 20:47, Mateusz Guzik <mjg@freebsd.org> wrote:
>>
>> The branch main has been updated by mjg:
>>
>> URL:
>> https://cgit.FreeBSD.org/src/commit/?id=e6bc24b0385a166526d20b2eb0cbb6a116350075
>>
>> commit e6bc24b0385a166526d20b2eb0cbb6a116350075
>> Author:     Mateusz Guzik <mjg@FreeBSD.org>
>> AuthorDate: 2022-12-13 20:42:32 +0000
>> Commit:     Mateusz Guzik <mjg@FreeBSD.org>
>> CommitDate: 2022-12-13 20:46:58 +0000
>>
>>     kref: switch internal type to atomic_t and bring back const to
>> kref_read
>>
>>     This unbreak drm-kmod build.
>>
>>     the const is part of Linux API
>>
>>     Unfortunately drm-kmod uses hand-rolled refcount* calls on a kref
>>     object. For now go the easy route of keeping it operational by
>> casting
>>     stuff internally.
>>
>>     The general goal here is to make FreeBSD refcount API use an opaque
>>     type, hence the ongoing removal of hand-rolled accesses.
>>
>>     Reported by:    emaste
>> ---
>>  sys/compat/linuxkpi/common/include/linux/kref.h | 22
>> +++++++++++-----------
>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/sys/compat/linuxkpi/common/include/linux/kref.h
>> b/sys/compat/linuxkpi/common/include/linux/kref.h
>> index 9a6814175223..5a1fd834a58d 100644
>> --- a/sys/compat/linuxkpi/common/include/linux/kref.h
>> +++ b/sys/compat/linuxkpi/common/include/linux/kref.h
>> @@ -44,35 +44,35 @@
>>
>>  struct kref {
>>         /* XXX In Linux this is a refcount_t */
>> -       volatile u_int32_t refcount;
>> +       atomic_t refcount;
>>  };
>>
>>  static inline void
>>  kref_init(struct kref *kref)
>>  {
>>
>> -       refcount_init(&kref->refcount, 1);
>> +       refcount_init((uint32_t *)&kref->refcount, 1);
>>  }
>>
>>  static inline unsigned int
>> -kref_read(struct kref *kref)
>> +kref_read(const struct kref *kref)
>>  {
>>
>> -       return (refcount_load(&kref->refcount));
>> +       return (refcount_load(__DECONST(u_int32_t *, &kref->refcount)));
>
> This is rather ugly IMO, can't we change refcount_load() to take a
> const pointer? It looks like the problem might be
> __atomic_load_int_relaxed(), but that could use const as well?

I agree it looks like crap, but I did not see a point in going down
that rabbit hole right now.

Note if one was to patch load_int, one should also patch all other
types for consistency. But then there is also kcsan variants to sort
out, making it into a rather big crunchfest for next to no gain from
my pov.

If anything I don't see why they added const in linux for this one.

>>  }
>>
>>  static inline void
>>  kref_get(struct kref *kref)
>>  {
>>
>> -       refcount_acquire(&kref->refcount);
>> +       refcount_acquire((uint32_t *)&kref->refcount);
>>  }
>>
>>  static inline int
>>  kref_put(struct kref *kref, void (*rel)(struct kref *kref))
>>  {
>>
>> -       if (refcount_release(&kref->refcount)) {
>> +       if (refcount_release((uint32_t *)&kref->refcount)) {
>>                 rel(kref);
>>                 return 1;
>>         }
>> @@ -84,7 +84,7 @@ kref_put_lock(struct kref *kref, void (*rel)(struct kref
>> *kref),
>>      spinlock_t *lock)
>>  {
>>
>> -       if (refcount_release(&kref->refcount)) {
>> +       if (refcount_release((uint32_t *)&kref->refcount)) {
>>                 spin_lock(lock);
>>                 rel(kref);
>>                 return (1);
>> @@ -98,7 +98,7 @@ kref_sub(struct kref *kref, unsigned int count,
>>  {
>>
>>         while (count--) {
>> -               if (refcount_release(&kref->refcount)) {
>> +               if (refcount_release((uint32_t *)&kref->refcount)) {
>>                         rel(kref);
>>                         return 1;
>>                 }
>> @@ -110,16 +110,16 @@ static inline int __must_check
>>  kref_get_unless_zero(struct kref *kref)
>>  {
>>
>> -       return refcount_acquire_if_not_zero(&kref->refcount);
>> +       return refcount_acquire_if_not_zero((uint32_t *)&kref->refcount);
>>  }
>>
>>  static inline int kref_put_mutex(struct kref *kref,
>>      void (*release)(struct kref *kref), struct mutex *lock)
>>  {
>>         WARN_ON(release == NULL);
>> -       if (unlikely(!refcount_release_if_not_last(&kref->refcount))) {
>> +       if (unlikely(!refcount_release_if_not_last((uint32_t
>> *)&kref->refcount))) {
>>                 mutex_lock(lock);
>> -               if (unlikely(!refcount_release(&kref->refcount))) {
>> +               if (unlikely(!refcount_release((uint32_t
>> *)&kref->refcount))) {
>>                         mutex_unlock(lock);
>>                         return 0;
>>                 }
>


-- 
Mateusz Guzik <mjguzik gmail.com>