From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
To: Greg KH <gregkh@linuxfoundation.org>, Shay Drory <shayd@nvidia.com>
Cc: <netdev@vger.kernel.org>, <pabeni@redhat.com>,
<davem@davemloft.net>, <kuba@kernel.org>, <edumazet@google.com>,
<david.m.ertman@intel.com>, <rafael@kernel.org>,
<ira.weiny@intel.com>, <linux-rdma@vger.kernel.org>,
<leon@kernel.org>, <tariqt@nvidia.com>,
Parav Pandit <parav@nvidia.com>
Subject: Re: [PATCH net-next v4 1/2] driver core: auxiliary bus: show auxiliary device IRQs
Date: Fri, 10 May 2024 16:01:01 +0200 [thread overview]
Message-ID: <ae6e151e-0c34-4ff8-a9f7-40e4cbdb9dee@intel.com> (raw)
In-Reply-To: <2024051038-compare-canon-4161@gregkh>
On 5/10/24 15:07, Greg KH wrote:
> On Fri, May 10, 2024 at 02:54:49PM +0200, Przemek Kitszel wrote:
>>>> +static ssize_t auxiliary_irq_mode_show(struct device *dev,
>>>> + struct device_attribute *attr, char *buf)
>>>> +{
>>>> + struct auxiliary_irq_info *info =
>>>> + container_of(attr, struct auxiliary_irq_info, sysfs_attr);
>>>> +
>>>> + if (refcount_read(xa_load(&irqs, info->irq)) > 1)
>>>
>>> refcount combined with xa? That feels wrong, why is refcount used for
>>> this at all?
>>
>> Not long ago I commented on similar usage for ice driver,
>> ~"since you are locking anyway this could be a plain counter",
>> and author replied
>> ~"additional semantics (like saturation) of refcount make me feel warm
>> and fuzzy" (sorry if misquoting too much).
>> That convinced me back then, so I kept quiet about that here.
>
> But why is this being incremented / decremented at all? What is that
> for?
[global]
This is just a counter, it is used to tell if given IRQ is shared or
exclusive. Hence there is a global xarray for that.
And my argument is for this case precisely.
[other]
There is also a separate xarray for each auxdev (IIRC) which is used as
generic dynamic container [that stores sysfs attrs], any other would
work (with different characteristics), but I see no problems with
picking xarray here.
>
>> The "use least powerful option" rule of thumb is perhaps more important.
>
> Yes, but use a refcount properly if needed, I can't figure out why a
> refcount is needed here at all, which is not a good sign.
>
>>>> + refcount_set(new_ref, 1);
>>>> + ref = __xa_cmpxchg(&irqs, irq, NULL, new_ref, GFP_KERNEL);
>>>> + if (ref) {
>>>> + kfree(new_ref);
>>>> + if (xa_is_err(ref)) {
>>>> + ret = xa_err(ref);
>>>> + goto out;
>>>> + }
>>>> +
>>>> + /* Another thread beat us to creating the enrtry. */
>>>> + refcount_inc(ref);
>>>
>>> How can that happen? Why not just use a normal simple lock for all of
>>> this so you don't have to mess with refcounts at all? This is not
>>> performance-relevent code at all, but yet with a refcount you cause
>>> almost the same issues that a normal lock would have, plus the increased
>>> complexity of all of the surrounding code (like this, and the crazy
>>> __xa_cmpxchg() call)
>>>
>>> Make this simple please.
>>
>> I find current API of xarray not ideal for this use case, and would like
>> to fix it, but let me write a proper RFC to don't derail (or slow down)
>> this series.
>
> Why do you need to use an xarray here at all? Why isn't this just tied
> directly to the aux device instead?
for [global] above I find xarray suitable soultion, for the [other] I'll
leave defending it to @Shay :)
>
> thanks,
>
> greg k-h
next prev parent reply other threads:[~2024-05-10 14:01 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-09 9:14 [PATCH net-next v4 0/2] Introduce auxiliary bus IRQs sysfs Shay Drory
2024-05-09 9:14 ` [PATCH net-next v4 1/2] driver core: auxiliary bus: show auxiliary device IRQs Shay Drory
2024-05-10 8:15 ` Greg KH
2024-05-10 12:54 ` Przemek Kitszel
2024-05-10 13:07 ` Greg KH
2024-05-10 14:01 ` Przemek Kitszel [this message]
2024-05-11 7:44 ` Greg KH
2024-05-12 7:30 ` Shay Drori
2024-05-12 15:32 ` Jason Gunthorpe
2024-05-13 8:33 ` Przemek Kitszel
2024-05-13 23:06 ` Jason Gunthorpe
2024-05-12 7:27 ` Shay Drori
2024-05-09 9:14 ` [PATCH net-next v4 2/2] net/mlx5: Expose SFs IRQs Shay Drory
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ae6e151e-0c34-4ff8-a9f7-40e4cbdb9dee@intel.com \
--to=przemyslaw.kitszel@intel.com \
--cc=davem@davemloft.net \
--cc=david.m.ertman@intel.com \
--cc=edumazet@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=ira.weiny@intel.com \
--cc=kuba@kernel.org \
--cc=leon@kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=parav@nvidia.com \
--cc=rafael@kernel.org \
--cc=shayd@nvidia.com \
--cc=tariqt@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).