| Message ID | 20221007132342.1590367-2-niklas.cassel@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | libata: misc frozen port cleanups | expand |
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, > > +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. 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? Thanks, John > +{ > + return ap->pflags & ATA_PFLAG_FROZEN; > +} > + > extern int ata_std_prereset(struct ata_link *link, unsigned long deadline); > extern int ata_wait_after_reset(struct ata_link *link, unsigned long deadline, > int (*check_ready)(struct ata_link *link));
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
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
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 :)
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; } +static inline bool ata_port_is_frozen(const struct ata_port *ap) +{ + return ap->pflags & ATA_PFLAG_FROZEN; +} + extern int ata_std_prereset(struct ata_link *link, unsigned long deadline); extern int ata_wait_after_reset(struct ata_link *link, unsigned long deadline, int (*check_ready)(struct ata_link *link));
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(+)