Fwd: [jlayton:mgtime 5/13] inode.c:undefined reference to `__invalid_cmpxchg_size'

Fwd: [jlayton:mgtime 5/13] inode.c:undefined reference to `__invalid_cmpxchg_size'

[Jeff Layton]

[-- Attachment #1: Type: text/plain, Size: 645 bytes --]

I've been getting some of these warning emails from the KTR. I think
this is in reference to this patch, which adds a 64-bit try_cmpxchg in
the timestamp handling code:

    https://lore.kernel.org/linux-fsdevel/20240708-mgtime-v4-0-a0f3c6fb57f3@kernel.org/

On m68k, there is a prototype for __invalid_cmpxchg_size, but no actual
function, AFAICT. Should that be defined somewhere, or is this a
deliberate way to force a build break in this case?

More to the point though: do I need to do anything special for m86k
here (or for other arches that can't do a native 64-bit cmpxchg)?

Thanks,
-- 
Jeff Layton <jlayton@kernel.org>

[-- Attachment #2: Forwarded message — [jlayton:mgtime 5/13] inode.c:undefined reference to `__invalid_cmpxchg_size' --]
[-- Type: message/rfc822, Size: 6453 bytes --]

From: kernel test robot <lkp@intel.com>
To: Jeff Layton <jlayton@kernel.org>
Cc: oe-kbuild-all@lists.linux.dev
Subject: [jlayton:mgtime 5/13] inode.c:undefined reference to `__invalid_cmpxchg_size'
Date: Tue, 9 Jul 2024 19:40:55 +0800
Message-ID: <202407091931.mztaeJHw-lkp@intel.com>

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/jlayton/linux.git mgtime
head:   81b2439edd7c9f966afcb091f414b7f219cda8f6
commit: 2265b64634f4af479ffb0c478409cfd56ec6dc4d [5/13] fs: add infrastructure for multigrain timestamps
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20240709/202407091931.mztaeJHw-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240709/202407091931.mztaeJHw-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407091931.mztaeJHw-lkp@intel.com/

All errors (new ones prefixed by >>):

   m68k-linux-ld: fs/inode.o: in function `inode_set_ctime_current':
>> inode.c:(.text+0x167a): undefined reference to `__invalid_cmpxchg_size'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [jlayton:mgtime 5/13] inode.c:undefined reference to `__invalid_cmpxchg_size'

[Geert Uytterhoeven]

Hi Jeff,

On Tue, Jul 9, 2024 at 1:58 PM Jeff Layton <jlayton@kernel.org> wrote:
> I've been getting some of these warning emails from the KTR. I think
> this is in reference to this patch, which adds a 64-bit try_cmpxchg in
> the timestamp handling code:
>
>     https://lore.kernel.org/linux-fsdevel/20240708-mgtime-v4-0-a0f3c6fb57f3@kernel.org/
>
> On m68k, there is a prototype for __invalid_cmpxchg_size, but no actual
> function, AFAICT. Should that be defined somewhere, or is this a
> deliberate way to force a build break in this case?

It's a deliberate way to break the build.

> More to the point though: do I need to do anything special for m86k
> here (or for other arches that can't do a native 64-bit cmpxchg)?

64-bit cmpxchg() is only guaranteed to exist on 64-bit platforms.
See also
https://elixir.bootlin.com/linux/latest/source/include/asm-generic/cmpxchg.h#L62

I think you can use arch_cmpxchg64(), though.

> ---------- Forwarded message ----------
> From: kernel test robot <lkp@intel.com>

>    m68k-linux-ld: fs/inode.o: in function `inode_set_ctime_current':
> >> inode.c:(.text+0x167a): undefined reference to `__invalid_cmpxchg_size'

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [jlayton:mgtime 5/13] inode.c:undefined reference to `__invalid_cmpxchg_size'

[Arnd Bergmann]

On Tue, Jul 9, 2024, at 15:45, Geert Uytterhoeven wrote:
> On Tue, Jul 9, 2024 at 1:58 PM Jeff Layton <jlayton@kernel.org> wrote:
>> I've been getting some of these warning emails from the KTR. I think
>> this is in reference to this patch, which adds a 64-bit try_cmpxchg in
>> the timestamp handling code:
>>
>>     https://lore.kernel.org/linux-fsdevel/20240708-mgtime-v4-0-a0f3c6fb57f3@kernel.org/
>>
>> On m68k, there is a prototype for __invalid_cmpxchg_size, but no actual
>> function, AFAICT. Should that be defined somewhere, or is this a
>> deliberate way to force a build break in this case?
>
> It's a deliberate way to break the build.
>
>> More to the point though: do I need to do anything special for m86k
>> here (or for other arches that can't do a native 64-bit cmpxchg)?
>
> 64-bit cmpxchg() is only guaranteed to exist on 64-bit platforms.
> See also
> https://elixir.bootlin.com/linux/latest/source/include/asm-generic/cmpxchg.h#L62
>
> I think you can use arch_cmpxchg64(), though.

arch_cmpxchg64() is an internal helper provided by some
architectures. Driver code should use cmpxchg64() for
the explicitly 64-bit sized atomic operation.

I'm fairly sure we still don't provide this across all
32-bit architectures though: on architectures that have
64-bit atomics (i686, armv6k, ...) these can be provided
architecture specific code, and on non-SMP kernels they
can use the generic fallback through
generic_cmpxchg64_local(), but on SMP architectures without
native atomics you need a Kconfig dependency to turn off
the particular code.

     Arnd

Re: [jlayton:mgtime 5/13] inode.c:undefined reference to `__invalid_cmpxchg_size'

[Jeff Layton]

On Tue, 2024-07-09 at 16:16 +0200, Arnd Bergmann wrote:
> On Tue, Jul 9, 2024, at 15:45, Geert Uytterhoeven wrote:
> > On Tue, Jul 9, 2024 at 1:58 PM Jeff Layton <jlayton@kernel.org>
> > wrote:
> > > I've been getting some of these warning emails from the KTR. I
> > > think
> > > this is in reference to this patch, which adds a 64-bit
> > > try_cmpxchg in
> > > the timestamp handling code:
> > > 
> > >    
> > > https://lore.kernel.org/linux-fsdevel/20240708-mgtime-v4-0-a0f3c6fb57f3@kernel.org/
> > > 
> > > On m68k, there is a prototype for __invalid_cmpxchg_size, but no
> > > actual
> > > function, AFAICT. Should that be defined somewhere, or is this a
> > > deliberate way to force a build break in this case?
> > 
> > It's a deliberate way to break the build.
> > 
> > > More to the point though: do I need to do anything special for
> > > m86k
> > > here (or for other arches that can't do a native 64-bit cmpxchg)?
> > 
> > 64-bit cmpxchg() is only guaranteed to exist on 64-bit platforms.
> > See also
> > https://elixir.bootlin.com/linux/latest/source/include/asm-generic/cmpxchg.h#L62
> > 
> > I think you can use arch_cmpxchg64(), though.
> 
> arch_cmpxchg64() is an internal helper provided by some
> architectures. Driver code should use cmpxchg64() for
> the explicitly 64-bit sized atomic operation.
> 
> I'm fairly sure we still don't provide this across all
> 32-bit architectures though: on architectures that have
> 64-bit atomics (i686, armv6k, ...) these can be provided
> architecture specific code, and on non-SMP kernels they
> can use the generic fallback through
> generic_cmpxchg64_local(), but on SMP architectures without
> native atomics you need a Kconfig dependency to turn off
> the particular code.
> 

I think the simplest solution is to make the floor value I'm tracking
be an atomic64_t. That looks like it should smooth over the differences
between arches. I'm testing a patch to do that now.

Thanks!
-- 
Jeff Layton <jlayton@kernel.org>

Re: [jlayton:mgtime 5/13] inode.c:undefined reference to `__invalid_cmpxchg_size'

[Arnd Bergmann]

On Tue, Jul 9, 2024, at 16:23, Jeff Layton wrote:
> On Tue, 2024-07-09 at 16:16 +0200, Arnd Bergmann wrote:
>> On Tue, Jul 9, 2024, at 15:45, Geert Uytterhoeven wrote:
>
> I think the simplest solution is to make the floor value I'm tracking
> be an atomic64_t. That looks like it should smooth over the differences
> between arches. I'm testing a patch to do that now.

Yes, atomic64_t should work, but be careful about using this
in a fast path since it can turn into a global spinlock
in lib/atomic64.c on architectures that don't support it
natively.

I'm still reading through the rest of your series, but
it appears that you pass the time value into 
ktime_to_timespec64() directly afterwards, so I guess
that is already a fairly large overhead on 32-bit
architectures and an extra spinlock doesn't hurt too
much.

Two more things I noticed in your patch:

- smp_load_acquire() on a 64-bit variable seems problematic
  as well, maybe this needs a spinlock on 32-bit
  architectures?

- for the coarse_ctime function, I think you should be
  able to avoid the conversion to timespec by just calling
  ktime_get_coarse_real_ts64() again instead of converting
  monotonic to real and then to timespec.

- inode_set_ctime_current() seems to now store a fine-grained
  timespec in the inode even for the !is_mgtime case, skipping
  the timestamp_truncate() step. This appears to potentially
  leak a non-truncated value to userspace, which would be
  inconsistent with the value read back from disk.

      Arnd

Re: [jlayton:mgtime 5/13] inode.c:undefined reference to `__invalid_cmpxchg_size'

[Jeff Layton]

On Tue, 2024-07-09 at 17:07 +0200, Arnd Bergmann wrote:
> On Tue, Jul 9, 2024, at 16:23, Jeff Layton wrote:
> > On Tue, 2024-07-09 at 16:16 +0200, Arnd Bergmann wrote:
> > > On Tue, Jul 9, 2024, at 15:45, Geert Uytterhoeven wrote:
> > 
> > I think the simplest solution is to make the floor value I'm
> > tracking
> > be an atomic64_t. That looks like it should smooth over the
> > differences
> > between arches. I'm testing a patch to do that now.
> 
> Yes, atomic64_t should work, but be careful about using this
> in a fast path since it can turn into a global spinlock
> in lib/atomic64.c on architectures that don't support it
> natively.
> 
> I'm still reading through the rest of your series, but
> it appears that you pass the time value into 
> ktime_to_timespec64() directly afterwards, so I guess
> that is already a fairly large overhead on 32-bit
> architectures and an extra spinlock doesn't hurt too
> much.
> 

Thanks Arnd.

The context for this is generally a write or other change to an inode,
so I too am hoping the overhead won't be too bad. It does take great
pains to avoid changing the ctime_floor value whenever possible.


> Two more things I noticed in your patch:
> 
> - smp_load_acquire() on a 64-bit variable seems problematic
>   as well, maybe this needs a spinlock on 32-bit
>   architectures?
> 

That should go away with the conversion of ctime_floor to atomic64_t.
AFAICT, arches that don't have native a 64-bit cmpxchg op usually
emulate that with hashed spinlocks. 

> - for the coarse_ctime function, I think you should be
>   able to avoid the conversion to timespec by just calling
>   ktime_get_coarse_real_ts64() again instead of converting
>   monotonic to real and then to timespec.
> 

Note that we might get different values for the coarse timestamps, but
if we do then the second fetch will just be a little later (which is
OK). I'll plan to make this change.


> - inode_set_ctime_current() seems to now store a fine-grained
>   timespec in the inode even for the !is_mgtime case, skipping
>   the timestamp_truncate() step. This appears to potentially
>   leak a non-truncated value to userspace, which would be
>   inconsistent with the value read back from disk.

Oof, you're right. I'll fix that up for the next version.

Thanks for the review!
-- 
Jeff Layton <jlayton@kernel.org>

Re: [jlayton:mgtime 5/13] inode.c:undefined reference to `__invalid_cmpxchg_size'

[Arnd Bergmann]

On Tue, Jul 9, 2024, at 17:27, Jeff Layton wrote:
> On Tue, 2024-07-09 at 17:07 +0200, Arnd Bergmann wrote:
>> On Tue, Jul 9, 2024, at 16:23, Jeff Layton wrote:
>
> The context for this is generally a write or other change to an inode,
> so I too am hoping the overhead won't be too bad. It does take great
> pains to avoid changing the ctime_floor value whenever possible.

Ok, I see. Have you considered hooking directly into the code
in kernel/time/timekeeping.c then? 

Since the coarse time is backed by the timekeeper that itself
is a cache of the current time, this would potentially avoid
some duplication:

- whenever the tk_core code gets updated, you can update
  the ctime_floor along with it, or integrate ctime_floor
  itself into the timekeeper

- you can use the same sequence count logic, either with the
  same &tk_core.seq or using a separate counter for the
  ctime updates

      Arnd

Re: [jlayton:mgtime 5/13] inode.c:undefined reference to `__invalid_cmpxchg_size'

[Jeff Layton]

On Tue, 2024-07-09 at 19:06 +0200, Arnd Bergmann wrote:
> On Tue, Jul 9, 2024, at 17:27, Jeff Layton wrote:
> > On Tue, 2024-07-09 at 17:07 +0200, Arnd Bergmann wrote:
> > > On Tue, Jul 9, 2024, at 16:23, Jeff Layton wrote:
> > 
> > The context for this is generally a write or other change to an
> > inode,
> > so I too am hoping the overhead won't be too bad. It does take
> > great
> > pains to avoid changing the ctime_floor value whenever possible.
> 
> Ok, I see. Have you considered hooking directly into the code
> in kernel/time/timekeeping.c then? 
> 
> Since the coarse time is backed by the timekeeper that itself
> is a cache of the current time, this would potentially avoid
> some duplication:
> 
> - whenever the tk_core code gets updated, you can update
>   the ctime_floor along with it, or integrate ctime_floor
>   itself into the timekeeper
> 
> - you can use the same sequence count logic, either with the
>   same &tk_core.seq or using a separate counter for the
>   ctime updates
> 


Yes, I had considered it on an earlier draft, but my attempt was pretty
laughable. You inspired me to take another look though...

If we go that route, what I think we'd want to do is add a new floor
value to the timekeeper and a couple of new functions:

ktime_get_coarse_floor - fetch the max of current coarse time and floor
ktime_get_fine_floor - fetch a fine-grained time and update the floor

The variety of different offsets inside the existing timekeeper code is
a bit bewildering, but I guess we'd want ktime_get_fine_floor to call
timekeeping_get_ns(&tk->tkr_mono) and keep the latest return cached.
When the coarse time is updated we'd zero out that cached floor value.

Updating that value in ktime_get_fine_floor will require locking or
(more likely) some sort of atomic op. timekeeping_get_ns returns u64
though, so I think we're still stuck needing to do a cmpxchg64.

If there is a way to cut down what we'd need to track to 32-bits or
less though, then that might become more appealing.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [jlayton:mgtime 5/13] inode.c:undefined reference to `__invalid_cmpxchg_size'

[Arnd Bergmann]

On Tue, Jul 9, 2024, at 20:27, Jeff Layton wrote:
> On Tue, 2024-07-09 at 19:06 +0200, Arnd Bergmann wrote:
>> On Tue, Jul 9, 2024, at 17:27, Jeff Layton wrote:
>> > On Tue, 2024-07-09 at 17:07 +0200, Arnd Bergmann wrote:
>
>
> Yes, I had considered it on an earlier draft, but my attempt was pretty
> laughable. You inspired me to take another look though...
>
> If we go that route, what I think we'd want to do is add a new floor
> value to the timekeeper and a couple of new functions:
>
> ktime_get_coarse_floor - fetch the max of current coarse time and floor
> ktime_get_fine_floor - fetch a fine-grained time and update the floor

I was thinking of keeping a name that is specific to the vfs
usage instead of the ktime_get_* namespace. I'm sure the timekeeping
maintainers will have an opinion on this though, one way or another.

> The variety of different offsets inside the existing timekeeper code is
> a bit bewildering, but I guess we'd want ktime_get_fine_floor to call
> timekeeping_get_ns(&tk->tkr_mono) and keep the latest return cached.
> When the coarse time is updated we'd zero out that cached floor value.

Why not update the cached value during the timekeeping update as well
instead of setting it to zero? That way you can just always use the
cached value for VFS and simplify the common code path for reading
that value.

> Updating that value in ktime_get_fine_floor will require locking or
> (more likely) some sort of atomic op. timekeeping_get_ns returns u64
> though, so I think we're still stuck needing to do a cmpxchg64.

Right, or atomic64_cmpxchg() to make it work on 32-bit.

     Arnd

Re: [jlayton:mgtime 5/13] inode.c:undefined reference to `__invalid_cmpxchg_size'

[Jeff Layton]

On Wed, 2024-07-10 at 10:38 +0200, Arnd Bergmann wrote:
> On Tue, Jul 9, 2024, at 20:27, Jeff Layton wrote:
> > On Tue, 2024-07-09 at 19:06 +0200, Arnd Bergmann wrote:
> > > On Tue, Jul 9, 2024, at 17:27, Jeff Layton wrote:
> > > > On Tue, 2024-07-09 at 17:07 +0200, Arnd Bergmann wrote:
> > 
> > 
> > Yes, I had considered it on an earlier draft, but my attempt was pretty
> > laughable. You inspired me to take another look though...
> > 
> > If we go that route, what I think we'd want to do is add a new floor
> > value to the timekeeper and a couple of new functions:
> > 
> > ktime_get_coarse_floor - fetch the max of current coarse time and floor
> > ktime_get_fine_floor - fetch a fine-grained time and update the floor
> 
> I was thinking of keeping a name that is specific to the vfs
> usage instead of the ktime_get_* namespace. I'm sure the timekeeping
> maintainers will have an opinion on this though, one way or another.
> 

Fair enough.

> > The variety of different offsets inside the existing timekeeper code is
> > a bit bewildering, but I guess we'd want ktime_get_fine_floor to call
> > timekeeping_get_ns(&tk->tkr_mono) and keep the latest return cached.
> > When the coarse time is updated we'd zero out that cached floor value.
> 
> Why not update the cached value during the timekeeping update as well
> instead of setting it to zero? That way you can just always use the
> cached value for VFS and simplify the common code path for reading
> that value.
> 

You mean just update it to the coarse time on the update? That seems
like it would also work.

> > Updating that value in ktime_get_fine_floor will require locking or
> > (more likely) some sort of atomic op. timekeeping_get_ns returns u64
> > though, so I think we're still stuck needing to do a cmpxchg64.
> 
> Right, or atomic64_cmpxchg() to make it work on 32-bit.
> 

I think that's the catch. Without being able to move to cmpxchg32 for
the floor update, we're not buying much by bringing it into the
timekeeper. Is there some big benefit that I'm missing?

-- 
Jeff Layton <jlayton@kernel.org>