[v2,14/15] xen/asm-generic: introduce stub header numa.h

Commit Message

Oleksii Kurochko Nov. 10, 2023, 4:30 p.m. UTC

<asm/numa.h> is common through some archs so it is moved
to asm-generic.

Signed-off-by: Oleksii Kurochko <[e-mail]>
---
Changes in V2:
	- update the commit message.
	- change u8 to uint8_t.
	- add ifnded CONFIG_NUMA.
---
 xen/include/asm-generic/numa.h | 40 ++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 xen/include/asm-generic/numa.h

Comments

Jan Beulich Nov. 15, 2023, 10:07 a.m. UTC | #1

On 10.11.2023 17:30, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/include/asm-generic/numa.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __ARCH_GENERIC_NUMA_H
> +#define __ARCH_GENERIC_NUMA_H
> +
> +#include <xen/types.h>
> +#include <xen/mm.h>

I'm afraid I can't spot what you need these for here. You clearly need
xen/stdint.h, and you need xen/mm-frame.h for mfn_t. Yes, max_page is
declared in xen/mm.h, but its questionable whether the header needs
including here for that reason, as all uses are in macros. (We aren't
anywhere near consistent in this regard). Plus you don't also include
xen/cpumask.h to pull in cpu_online_map (which another macro
references).

> +typedef uint8_t nodeid_t;
> +
> +#ifndef CONFIG_NUMA

Isn't it an error to include this header when NUMA=y?

> +/* Fake one node for now. See also node_online_map. */
> +#define cpu_to_node(cpu) 0
> +#define node_to_cpumask(node)   (cpu_online_map)
> +
> +/*
> + * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
> + * is required because the dummy helpers are using it.
> + */
> +extern mfn_t first_valid_mfn;
> +
> +/* XXX: implement NUMA support */
> +#define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
> +#define node_start_pfn(nid) (mfn_x(first_valid_mfn))
> +#define __node_distance(a, b) (20)
> +
> +#endif
> +
> +#define arch_want_default_dmazone() (false)
> +
> +#endif /* __ARCH_GENERIC_NUMA_H */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

Oleksii Kurochko Nov. 15, 2023, 2:11 p.m. UTC | #2

On Wed, 2023-11-15 at 11:07 +0100, Jan Beulich wrote:
> On 10.11.2023 17:30, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/include/asm-generic/numa.h
> > @@ -0,0 +1,40 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __ARCH_GENERIC_NUMA_H
> > +#define __ARCH_GENERIC_NUMA_H
> > +
> > +#include <xen/types.h>
> > +#include <xen/mm.h>
> 
> I'm afraid I can't spot what you need these for here. You clearly
> need
> xen/stdint.h, and you need xen/mm-frame.h for mfn_t. Yes, max_page is
> declared in xen/mm.h, but its questionable whether the header needs
> including here for that reason, as all uses are in macros. (We aren't
> anywhere near consistent in this regard). Plus you don't also include
> xen/cpumask.h to pull in cpu_online_map (which another macro
> references).
I agree with almost you wrote but should <xen/cpumas.h> be included
then? It looks like it is the same situation as with max_page and
<xen/mm.h>.

> 
> > +typedef uint8_t nodeid_t;
> > +
> > +#ifndef CONFIG_NUMA
> 
> Isn't it an error to include this header when NUMA=y?
It's still need to define arch_want_default_dmazone() macros which is
used by common code.
All other code is under #ifndef CONFIG_NUMA so it won't conflict with
defintions in <xen/numa.h>.

> 
> > +/* Fake one node for now. See also node_online_map. */
> > +#define cpu_to_node(cpu) 0
> > +#define node_to_cpumask(node)   (cpu_online_map)
> > +
> > +/*
> > + * TODO: make first_valid_mfn static when NUMA is supported on
> > Arm, this
> > + * is required because the dummy helpers are using it.
> > + */
> > +extern mfn_t first_valid_mfn;
> > +
> > +/* XXX: implement NUMA support */
> > +#define node_spanned_pages(nid) (max_page -
> > mfn_x(first_valid_mfn))
> > +#define node_start_pfn(nid) (mfn_x(first_valid_mfn))
> > +#define __node_distance(a, b) (20)
> > +
> > +#endif
> > +
> > +#define arch_want_default_dmazone() (false)
> > +
> > +#endif /* __ARCH_GENERIC_NUMA_H */
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> 

~ Oleksii

Jan Beulich Nov. 16, 2023, 7:31 a.m. UTC | #3

On 15.11.2023 15:11, Oleksii wrote:
> On Wed, 2023-11-15 at 11:07 +0100, Jan Beulich wrote:
>> On 10.11.2023 17:30, Oleksii Kurochko wrote:
>>> --- /dev/null
>>> +++ b/xen/include/asm-generic/numa.h
>>> @@ -0,0 +1,40 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +#ifndef __ARCH_GENERIC_NUMA_H
>>> +#define __ARCH_GENERIC_NUMA_H
>>> +
>>> +#include <xen/types.h>
>>> +#include <xen/mm.h>
>>
>> I'm afraid I can't spot what you need these for here. You clearly
>> need
>> xen/stdint.h, and you need xen/mm-frame.h for mfn_t. Yes, max_page is
>> declared in xen/mm.h, but its questionable whether the header needs
>> including here for that reason, as all uses are in macros. (We aren't
>> anywhere near consistent in this regard). Plus you don't also include
>> xen/cpumask.h to pull in cpu_online_map (which another macro
>> references).
> I agree with almost you wrote but should <xen/cpumas.h> be included
> then? It looks like it is the same situation as with max_page and
> <xen/mm.h>.

Well, yes and no: Indeed the minimal requirement is to be consistent
(either include both or include neither). Personally I'd prefer if
headers would be included only if they are needed to successfully
compiler the header on its own. That limits needless dependencies on
the (otherwise) included headers. But I can easily see that others
might take the other sensible perspective.

>>> +typedef uint8_t nodeid_t;
>>> +
>>> +#ifndef CONFIG_NUMA
>>
>> Isn't it an error to include this header when NUMA=y?
> It's still need to define arch_want_default_dmazone() macros which is
> used by common code.

Oh, yes. I somehow managed to overlook that. Some of the #include-s then
want to move inside the #ifndef, though. (Ultimately I question the
placement of this #define in this header, but we can deal with that
separately.)

Jan

Patch

diff --git a/xen/include/asm-generic/numa.h b/xen/include/asm-generic/numa.h
new file mode 100644
index 0000000000..353642c353
--- /dev/null
+++ b/xen/include/asm-generic/numa.h
@@ -0,0 +1,40 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __ARCH_GENERIC_NUMA_H
+#define __ARCH_GENERIC_NUMA_H
+
+#include <xen/types.h>
+#include <xen/mm.h>
+
+typedef uint8_t nodeid_t;
+
+#ifndef CONFIG_NUMA
+
+/* Fake one node for now. See also node_online_map. */
+#define cpu_to_node(cpu) 0
+#define node_to_cpumask(node)   (cpu_online_map)
+
+/*
+ * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
+ * is required because the dummy helpers are using it.
+ */
+extern mfn_t first_valid_mfn;
+
+/* XXX: implement NUMA support */
+#define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
+#define node_start_pfn(nid) (mfn_x(first_valid_mfn))
+#define __node_distance(a, b) (20)
+
+#endif
+
+#define arch_want_default_dmazone() (false)
+
+#endif /* __ARCH_GENERIC_NUMA_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */