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

[John Garry <john.garry@huawei.com>]

On 10/10/2022 11:17, Niklas Cassel wrote:

Hi Niklas,

> 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.

I was just checking *ata_port, and based my judgement on that one.

> 
> 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.
> 

sure

> 
> "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.

yeah, I think it should be fine, as the compiler should treat 
ata_port_is_frozen() as self-contained and thus make no judgement 
optimizing out such reads when inlining.

> 
> 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:)

I'm still not inclined to add const specifier, but I'll leave that to 
Damien and you.

I think generally we could add const a lot more in the kernel codebase. 
But we don't, as if we need to change how we treat the data we point to, 
i.e. stop writing or start writing, then we need to start changing APIs, 
and that is not so welcome, and so generally omit it. The same goes for 
non-ptr functions args. And then programmers are often a bit lazy and 
over-confident too (to not bother using it) :).

Thanks,
John