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

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 2022/10/10 20:22, John Garry wrote:
> 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.

Given that this helper is clearly intends to only read the port flags, I am fine
with the const argument, even though I think this will not buy us anything from
the compiler given the simplicity of the function :)

-- 
Damien Le Moal
Western Digital Research