Re: [PATCH 1/4] ata: add ata_port_is_frozen() helper

[Niklas Cassel <Niklas.Cassel@wdc.com>]

On Mon, Oct 10, 2022 at 08:00:09AM +0100, John Garry wrote:
> On 07/10/2022 14:23, Niklas Cassel wrote:
> > At the request of the libata maintainer, introduce a ata_port_is_frozen()
> > helper function.
> > 
> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > ---
> >   include/linux/libata.h | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/include/linux/libata.h b/include/linux/libata.h
> > index a505cfb92ab3..d5ac52654b42 100644
> > --- a/include/linux/libata.h
> > +++ b/include/linux/libata.h
> > @@ -1043,6 +1043,11 @@ static inline int ata_port_is_dummy(struct ata_port *ap)
> >   	return ap->ops == &ata_dummy_port_ops;
> >   }
> 
> Hi Niklas,

Hello John,

> 
> >
> > +static inline bool ata_port_is_frozen(const struct ata_port *ap)
> 
> The majority of libata APIs don't use const in this way, so I think that
> consistency is better.

Well, right now there is no consistency :)

$ git grep "static inline" include/linux/libata.h | grep "(const struct"
include/linux/libata.h:static inline bool ata_port_is_frozen(const struct ata_port *ap)
include/linux/libata.h:static inline int ata_acpi_stm(const struct ata_port *ap,
include/linux/libata.h:static inline int ata_acpi_gtm(const struct ata_port *ap,
include/linux/libata.h:static inline bool ata_is_host_link(const struct ata_link *link)
include/linux/libata.h:static inline bool ata_is_host_link(const struct ata_link *link)
include/linux/libata.h:static inline unsigned int ata_dev_enabled(const struct ata_device *dev)
include/linux/libata.h:static inline unsigned int ata_dev_disabled(const struct ata_device *dev)
include/linux/libata.h:static inline unsigned int ata_dev_absent(const struct ata_device *dev)
include/linux/libata.h:static inline int ata_link_max_devices(const struct ata_link *link)
include/linux/libata.h:static inline int ata_try_flush_cache(const struct ata_device *dev)

There are 10 uses (9 without my addition) that uses a const struct pointer.

So since both are used in libata, I chose the one that seemed most correct.

> 
> Indeed, this is not const data which you're pointing at, so maybe it's
> better to be honest with the compiler. And since this is inlined, could the
> compiler optimise out multiple reads on ap->flags in a caller function since
> we tell it it's const?

"This is not const data which you're pointing at"

Well, according to 6.7.6.1 Pointer declarators in
https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3054.pdf

A "const struct *ptr" means that the contents of any object pointed to
cannot be modified through that pointer.


"And since this is inlined, could the compiler optimise out multiple reads
on ap->flags in a caller function since we tell it it's const?"

I'm far from a compiler expert, but because an optimising compiler is free
to inline whatever function it wants, not just functions marked inline,
I would assume that the compiler would "do the right thing" regardless if
a function is marked as inline or not.

Doing a:
git grep "static inline" include/ | grep "(const struct" | wc -l
2055

Makes me quite confident that this should be fine.
Sure, the data it points to might never change.

But seeing e.g.:
$ git grep "static inline" include/ | grep "empty(const struct"

Especially used in tcp and qdisc makes me even more confident that this
will work fine.

Looking at e.g. __dev_xmit_skb():
https://elixir.bootlin.com/linux/v6.0/source/net/core/dev.c#L3803
we can see that it uses nolock_qdisc_is_empty() multiple times within
the same function. So now I'm very confident that this will be fine :)


Kind regards,
Niklas