Re: [PATCH v5 13/23] xen/riscv: introduce atomic.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.03.2024 14:30, Oleksii wrote:
> On Wed, 2024-03-06 at 16:31 +0100, Jan Beulich wrote:
>> On 26.02.2024 18:38, Oleksii Kurochko wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/riscv/include/asm/atomic.h
>>> @@ -0,0 +1,296 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Taken and modified from Linux.
>>> + *
>>> + * The following changes were done:
>>> + * - * atomic##prefix##_*xchg_*(atomic##prefix##_t *v, c_t n) were
>>> updated
>>> + *     to use__*xchg_generic()
>>> + * - drop casts in write_atomic() as they are unnecessary
>>> + * - drop introduction of WRITE_ONCE() and READ_ONCE().
>>> + *   Xen provides ACCESS_ONCE()
>>> + * - remove zero-length array access in read_atomic()
>>> + * - drop defines similar to pattern
>>> + *   #define atomic_add_return_relaxed   atomic_add_return_relaxed
>>> + * - move not RISC-V specific functions to asm-generic/atomics-
>>> ops.h
>>> + * 
>>> + * Copyright (C) 2007 Red Hat, Inc. All Rights Reserved.
>>> + * Copyright (C) 2012 Regents of the University of California
>>> + * Copyright (C) 2017 SiFive
>>> + * Copyright (C) 2024 Vates SAS
>>> + */
>>> +
>>> +#ifndef _ASM_RISCV_ATOMIC_H
>>> +#define _ASM_RISCV_ATOMIC_H
>>> +
>>> +#include <xen/atomic.h>
>>> +
>>> +#include <asm/cmpxchg.h>
>>> +#include <asm/fence.h>
>>> +#include <asm/io.h>
>>> +#include <asm/system.h>
>>> +
>>> +#include <asm-generic/atomic-ops.h>
>>
>> While, because of the forward decls in xen/atomic.h, having this
>> #include
>> works, I wonder if it wouldn't better be placed further down. The
>> compiler
>> will likely have an easier time when it sees the inline definitions
>> ahead
>> of any uses.
> Do you mean to move it after #define __atomic_release_fence() ?

Perhaps yet further down, at least after the arithmetic ops were defined.

>>> --- /dev/null
>>> +++ b/xen/include/asm-generic/atomic-ops.h
>>> @@ -0,0 +1,92 @@
>>> +#/* SPDX-License-Identifier: GPL-2.0 */
>>> +#ifndef _ASM_GENERIC_ATOMIC_OPS_H_
>>> +#define _ASM_GENERIC_ATOMIC_OPS_H_
>>> +
>>> +#include <xen/atomic.h>
>>> +#include <xen/lib.h>
>>
>> If I'm not mistaken this header provides default implementations for
>> every
>> xen/atomic.h-provided forward inline declaration that can be
>> synthesized
>> from other atomic functions. I think a comment to this effect would
>> want
>> adding somewhere here.
> I think we can drop this inclusion here as inclusion of asm-
> generic/atomic-ops.h will be always go with inclusion of xen/atomic.h.

I'm okay with dropping that include, but that wasn't the purpose of my
comment. I was rather asking for a comment to be added here stating
what is (not) to be present in this header.

Jan