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
