[PRELIMINARY] headers: Refactor kernel_structs.h and introduce kernel_arch.h. #20047

stephanosio · 2019-10-23T09:34:32Z

This commit refactors kernel_structs.h to be more truthful to its name
(i.e. define kernel structures only), and introduces kernel_arch.h to
provide the definitions that are not kernel structure related and were
previously provided by kernel_structs.h.

In addition, this commit updates all required inclusions of
kernel_structs.h to use kernel_arch.h instead and makes the necessary
adjustments to resolve header dependencies.

The primary reason for refactoring kernel_structs.h is to remove its
dependency on kernel.h so that the kernel structure definitions
provided by it can be used in arch headers (for instance, in
include/arch/arm/irq.h). This was not possible until now due to a
circular dependency of these headers as described in the issue #3056.

Signed-off-by: Stephanos Ioannidis [email protected]

This fixes #3056.

This commit refactors kernel_structs.h to be more truthful to its name (i.e. define kernel structures only), and introduces kernel_arch.h to provide the definitions that are not kernel structure related and were previously provided by kernel_structs.h. In addition, this commit updates all required inclusions of kernel_structs.h to use kernel_arch.h instead and makes the necessary adjustments to resolve header dependencies. The primary reason for refactoring kernel_structs.h is to remove its dependency on kernel.h so that the kernel structure definitions provided by it can be used in arch headers (for instance, in include/arch/arm/irq.h). This was not possible until now due to a circular dependency of these headers as described in the issue zephyrproject-rtos#3056. Signed-off-by: Stephanos Ioannidis <[email protected]>

stephanosio · 2019-10-23T14:38:12Z

NOTE: Once this gets merged, the following hacks in arch/arm and arch/x86 should be fixed:

zephyr/include/arch/x86/ia32/arch.h

Line 238 in cc1b94c

/* FIXME prefer these inline, but see GH-3056 */

zephyr/arch/x86/core/ia32/irq_manage.c

Lines 41 to 48 in 8ffff14

    
           /* FIXME: IRQ direct inline functions have to be placed here and not in 
        
            * arch/cpu.h as inline functions due to nasty circular dependency between 
        
            * arch/cpu.h and kernel_structs.h; the inline functions typically need to 
        
            * perform operations on _kernel.  For now, leave as regular functions, a 
        
            * future iteration will resolve this. 
        
            * 
        
            * See https://github.com/zephyrproject-rtos/zephyr/issues/3056 
        
            */

zephyr/include/arch/arm/irq.h

Line 93 in 8ffff14

/* FIXME prefer these inline, but see GH-3056 */

zephyr/arch/arm/core/irq_manage.c

Lines 160 to 167 in 8ffff14

    
           /* FIXME: IRQ direct inline functions have to be placed here and not in 
        
            * arch/cpu.h as inline functions due to nasty circular dependency between 
        
            * arch/cpu.h and kernel_structs.h; the inline functions typically need to 
        
            * perform operations on _kernel.  For now, leave as regular functions, a 
        
            * future iteration will resolve this. 
        
            * 
        
            * See https://github.com/zephyrproject-rtos/zephyr/issues/3056 
        
            */

ioannisg · 2019-10-23T14:54:07Z

Quick comment; you could use the DRAFT pull-request feature for "preliminary" work, or add work-in-progress label.

This seems to be fixing an old issue - I'd like to take a look

andyross

IIRC, the original reason for having kernel_structs.h was to isolate the data structure declarations from the inline functions that used them and to have a framework to better manage the crazy header dependencies. And then it rotted a little, I guess. This looks like a nice return to sanity to me.

andyross · 2019-10-23T15:18:56Z

kernel/include/kernel_arch.h

+			      unsigned int options);
+
+static ALWAYS_INLINE void z_new_thread_init(struct k_thread *thread,
+					    char *pStack, size_t stackSize,


Seems like this one probably wants to go into a C file somewhere, there's little value to having an inlined thread initialization routine. Not really in scope of this patch though.

yes please move this to a C function, prototype it in kernel_internal.h or something

stephanosio · 2019-10-23T15:35:03Z

Could someone take a look at posix and arc? They are broken at the moment and, for some reason, my local build system is refusing to build them.

andrewboie

It's good to have someone looking into this.

Also you may not be aware, but right now we have an issue with our header include paths. arch/*/include and kernel/include are currently in the default include path, but they should not be, they are for definitions private to the kernel itself.

please see #19666

andrewboie · 2019-10-23T16:19:56Z

kernel/include/kernel_arch.h

+			      unsigned int options);
+
+static ALWAYS_INLINE void z_new_thread_init(struct k_thread *thread,
+					    char *pStack, size_t stackSize,


yes please move this to a C function, prototype it in kernel_internal.h or something

andrewboie · 2019-10-23T16:20:29Z

kernel/include/kernel_arch.h

+ * z_swap() is in use it's a simple inline provided by the kernel.
+ */
+static ALWAYS_INLINE void
+z_arch_thread_return_value_set(struct k_thread *thread, unsigned int value)


duplicate definition.
this is already defined in include/sys/arch_interface.h

well, the one in include/sys/arch_interface.h does not include implementation of it.

that's because it gets defined by the arch code, typically in arch/*/include/kernel_arch_func.h

everything defined in include/sys/arch_interface.h has its implementation in arch-specific code, there's no general implementation for them

everything defined in include/sys/arch_interface.h has its implementation in arch-specific code, there's no general implementation for them

Yes, but the point of this code is to provide implementation of it when CONFIG_USE_SWITCH is enabled, in which case arch-specific code should not provide an implementation (at least, according to the comment here):

zephyr/kernel/include/kernel_structs.h

Lines 194 to 203 in 9784f80

#ifdef CONFIG_USE_SWITCH

/* This is a arch function traditionally, but when the switch-based

* z_swap() is in use it's a simple inline provided by the kernel.

*/

static ALWAYS_INLINE void

z_arch_thread_return_value_set(struct k_thread *thread, unsigned int value)

{

thread->swap_retval = value;

}

#endif

https://github.com/zephyrproject-rtos/zephyr/pull/20047/files/426c6cad08f7869f734bb3a8c3d681aa0c7b2e2a#diff-7d60221bd6a21c05f5f30c3d0ff07e7f

Note that this code was simply moved out of kernel_structs.h to kernel_arch.h (as it certainly does not belong in kernel_structs.h).

Just to be sure, I further validate this as follows:

In k_thread, swap_retval field is added when CONFIG_USE_SWITCH is enabled.

zephyr/include/kernel.h

Lines 574 to 584 in 9784f80

#if defined(CONFIG_USE_SWITCH)

/* When using __switch() a few previously arch-specific items

* become part of the core OS

*/

/** z_swap() return value */

int swap_retval;

/** Context handle returned via z_arch_switch() */

void *switch_handle;

#endif

do_swap uses k_thread swap_retval as the thread return value.

zephyr/kernel/include/kswap.h

Lines 69 to 93 in 9784f80

old_thread->swap_retval = -EAGAIN;

#ifdef CONFIG_SMP

_current_cpu->swap_ok = 0;

new_thread->base.cpu = z_arch_curr_cpu()->id;

if (!is_spinlock) {

z_smp_release_global_lock(new_thread);

}

#endif

_current = new_thread;

z_arch_switch(new_thread->switch_handle,

&old_thread->switch_handle);

}

sys_trace_thread_switched_in();

if (is_spinlock) {

z_arch_irq_unlock(key);

} else {

irq_unlock(key);

}

return _current->swap_retval;

the way to solve this is to add another layer of indirection.

define a z_thread_return_value_set(), which either calls into the arch API or sets the thread member depending on whether SWITCH is enabled

the way to solve this is to add another layer of indirection.

Yes, that would be the right way to do it. I am somewhat reluctant to introduce that change in this PR though due to the possibility of bringing on yet another dependency hell, which this PR already has more than enough of.

Maybe it could be addressed in another upcoming arch interface-related PR.

andrewboie · 2019-10-23T16:24:35Z

kernel/include/kernel_structs.h

@@ -180,6 +177,8 @@ typedef struct z_kernel _kernel_t;
 extern struct z_kernel _kernel;

 #ifdef CONFIG_SMP
+static inline struct _cpu *z_arch_curr_cpu(void);


this is also something that is defined in arch_interface.h

I haven't fully analysed it, but the point of it is to provide a forward reference to the function, as there seemed to be some files that include kernel_structs.h first, then have some code that uses z_arch_curr_cpu, and then include arch_interface.h (yes, it is a big mess). It helps to get rid of quite a few implicit declaration warnings.

I can't get behind a patch which un-does the consolidation I recently spent quite some time doing for our arch headers.

as there seemed to be some files that include kernel_structs.h first, then have some code that uses z_arch_curr_cpu, and then include arch_interface.h

That's what needs to be fixed.
Is this code in arch/ space or within the kernel?

I don't remember exactly which one it was because it came up while running hundreds of sanity tests.

I will check if I can pin-point this tomorrow.

andrewboie · 2019-10-23T16:26:19Z

kernel/include/kernel_arch.h

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#ifndef ZEPHYR_KERNEL_INCLUDE_KERNEL_ARCH_H_


why did you decide to call this kernel_arch.h?

What is the intended purpose of this header? I'd rather you called this something else, we already have headers for the kernel code to arch interface. It also appears you have some duplicate definitions in here.

You can however, move include/sys/arch_interface.h to kernel/arch_interface.h if you like.

kernel_arch.h is really just something I added to achieve what this PR intends to do (remove all the junk from kernel_structs.h so that it can be included in arch headers), with the least amount of refactoring involved.

I know this may not be the best solution in terms of the long term header clean-up strategy, but at least for now, this seems to be a prominent solution for achieving the main goal of this PR.

I will review #19666 and see if I can come up with something better tomorrow.

stephanosio · 2019-10-24T01:46:40Z

This PR is on hold for now. After reading #19666, given that this PR already requires many repository-wide modifications, I thought it would make more sense to do the proper refactoring as per #19666 and do all the modifications at once.

stephanosio · 2019-10-25T02:19:34Z

Closing as the changes in #20119 supersede this.

#20119 addresses #3056 and #19666.

This commit refactors kernel and arch headers to establish a boundary between private and public interface headers. The refactoring strategy used in this commit is detailed in the issue This commit introduces the following major changes: 1. Establish a clear boundary between private and public headers by removing "kernel/include" and "arch/*/include" from the global include paths. Ideally, only kernel/ and arch/*/ source files should reference the headers in these directories. If these headers must be used by a component, these include paths shall be manually added to the CMakeLists.txt file of the component. This is intended to discourage applications from including private kernel and arch headers either knowingly and unknowingly. - kernel/include/ (PRIVATE) This directory contains the private headers that provide private kernel definitions which should not be visible outside the kernel and arch source code. All public kernel definitions must be added to an appropriate header located under include/. - arch/*/include/ (PRIVATE) This directory contains the private headers that provide private architecture-specific definitions which should not be visible outside the arch and kernel source code. All public architecture- specific definitions must be added to an appropriate header located under include/arch/*/. - include/ AND include/sys/ (PUBLIC) This directory contains the public headers that provide public kernel definitions which can be referenced by both kernel and application code. - include/arch/*/ (PUBLIC) This directory contains the public headers that provide public architecture-specific definitions which can be referenced by both kernel and application code. 2. Split arch_interface.h into "kernel-to-arch interface" and "public arch interface" divisions. - kernel/include/kernel_arch_interface.h * provides private "kernel-to-arch interface" definition. * includes arch/*/include/kernel_arch_func.h to ensure that the interface function implementations are always available. * includes sys/arch_interface.h so that public arch interface definitions are automatically included when including this file. - arch/*/include/kernel_arch_func.h * provides architecture-specific "kernel-to-arch interface" implementation. * only the functions that will be used in kernel and arch source files are defined here. - include/sys/arch_interface.h * provides "public arch interface" definition. * includes include/arch/arch_inlines.h to ensure that the architecture-specific public inline interface function implementations are always available. - include/arch/arch_inlines.h * includes architecture-specific arch_inlines.h in include/arch/*/arch_inline.h. - include/arch/*/arch_inline.h * provides architecture-specific "public arch interface" inline function implementation. * supersedes include/sys/arch_inline.h. 3. Refactor kernel and the existing architecture implementations. - Remove circular dependency of kernel and arch headers. The following general rules should be observed: * Never include any private headers from public headers * Never include kernel_internal.h in kernel_arch_data.h * Always include kernel_arch_data.h from kernel_arch_func.h * Never include kernel.h from kernel_struct.h either directly or indirectly. Only add the kernel structures that must be referenced from public arch headers in this file. - Relocate syscall_handler.h to include/ so it can be used in the public code. This is necessary because many user-mode public codes reference the functions defined in this header. - Relocate kernel_arch_thread.h to include/arch/*/thread.h. This is necessary to provide architecture-specific thread definition for 'struct k_thread' in kernel.h. - Remove any private header dependencies from public headers using the following methods: * If dependency is not required, simply omit * If dependency is required, - Relocate a portion of the required dependencies from the private header to an appropriate public header OR - Relocate the required private header to make it public. This commit supersedes zephyrproject-rtos#20047, addresses zephyrproject-rtos#19666, and fixes zephyrproject-rtos#3056. Signed-off-by: Stephanos Ioannidis <[email protected]>

This commit refactors kernel and arch headers to establish a boundary between private and public interface headers. The refactoring strategy used in this commit is detailed in the issue This commit introduces the following major changes: 1. Establish a clear boundary between private and public headers by removing "kernel/include" and "arch/*/include" from the global include paths. Ideally, only kernel/ and arch/*/ source files should reference the headers in these directories. If these headers must be used by a component, these include paths shall be manually added to the CMakeLists.txt file of the component. This is intended to discourage applications from including private kernel and arch headers either knowingly and unknowingly. - kernel/include/ (PRIVATE) This directory contains the private headers that provide private kernel definitions which should not be visible outside the kernel and arch source code. All public kernel definitions must be added to an appropriate header located under include/. - arch/*/include/ (PRIVATE) This directory contains the private headers that provide private architecture-specific definitions which should not be visible outside the arch and kernel source code. All public architecture- specific definitions must be added to an appropriate header located under include/arch/*/. - include/ AND include/sys/ (PUBLIC) This directory contains the public headers that provide public kernel definitions which can be referenced by both kernel and application code. - include/arch/*/ (PUBLIC) This directory contains the public headers that provide public architecture-specific definitions which can be referenced by both kernel and application code. 2. Split arch_interface.h into "kernel-to-arch interface" and "public arch interface" divisions. - kernel/include/kernel_arch_interface.h * provides private "kernel-to-arch interface" definition. * includes arch/*/include/kernel_arch_func.h to ensure that the interface function implementations are always available. * includes sys/arch_interface.h so that public arch interface definitions are automatically included when including this file. - arch/*/include/kernel_arch_func.h * provides architecture-specific "kernel-to-arch interface" implementation. * only the functions that will be used in kernel and arch source files are defined here. - include/sys/arch_interface.h * provides "public arch interface" definition. * includes include/arch/arch_inlines.h to ensure that the architecture-specific public inline interface function implementations are always available. - include/arch/arch_inlines.h * includes architecture-specific arch_inlines.h in include/arch/*/arch_inline.h. - include/arch/*/arch_inline.h * provides architecture-specific "public arch interface" inline function implementation. * supersedes include/sys/arch_inline.h. 3. Refactor kernel and the existing architecture implementations. - Remove circular dependency of kernel and arch headers. The following general rules should be observed: * Never include any private headers from public headers * Never include kernel_internal.h in kernel_arch_data.h * Always include kernel_arch_data.h from kernel_arch_func.h * Never include kernel.h from kernel_struct.h either directly or indirectly. Only add the kernel structures that must be referenced from public arch headers in this file. - Relocate syscall_handler.h to include/ so it can be used in the public code. This is necessary because many user-mode public codes reference the functions defined in this header. - Relocate kernel_arch_thread.h to include/arch/*/thread.h. This is necessary to provide architecture-specific thread definition for 'struct k_thread' in kernel.h. - Remove any private header dependencies from public headers using the following methods: * If dependency is not required, simply omit * If dependency is required, - Relocate a portion of the required dependencies from the private header to an appropriate public header OR - Relocate the required private header to make it public. This commit supersedes #20047, addresses #19666, and fixes #3056. Signed-off-by: Stephanos Ioannidis <[email protected]>

stephanosio requested review from andrewboie, andyross, galak, ioannisg and MaureenHelm as code owners October 23, 2019 09:34

stephanosio added the area: Kernel label Oct 23, 2019

zephyrbot added the area: ARM ARM (32-bit) Architecture label Oct 23, 2019

stephanosio changed the title ~~headers: Refactor kernel_structs.h and introduce kernel_arch.h.~~ [PRELIMINARY] headers: Refactor kernel_structs.h and introduce kernel_arch.h. Oct 23, 2019

stephanosio force-pushed the introduce_kernel_arch_h branch from 2137dce to 426c6ca Compare October 23, 2019 14:34

stephanosio requested review from carlescufi and dcpleung as code owners October 23, 2019 14:34

stephanosio requested a review from a user October 23, 2019 14:34

stephanosio requested review from kgugala, nashif, nategraff-sifive, pgielda and wentongwu as code owners October 23, 2019 14:34

zephyrbot added area: X86 x86 Architecture (32-bit) area: NIOS2 NIOS2 Architecture area: Xtensa Xtensa Architecture area: API Changes to public APIs area: Tests Issues related to a particular existing or missing test labels Oct 23, 2019

stephanosio added the In progress For PRs: is work in progress and should not be merged yet. For issues: Is being worked on label Oct 23, 2019

andyross approved these changes Oct 23, 2019

View reviewed changes

stephanosio added the DNM This PR should not be merged (Do Not Merge) label Oct 23, 2019

andrewboie suggested changes Oct 23, 2019

View reviewed changes

stephanosio mentioned this pull request Oct 24, 2019

headers: Refactor kernel and arch headers. #20119

Merged

stephanosio closed this Oct 25, 2019

stephanosio deleted the introduce_kernel_arch_h branch October 31, 2019 18:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PRELIMINARY] headers: Refactor kernel_structs.h and introduce kernel_arch.h. #20047

[PRELIMINARY] headers: Refactor kernel_structs.h and introduce kernel_arch.h. #20047

stephanosio commented Oct 23, 2019

stephanosio commented Oct 23, 2019

ioannisg commented Oct 23, 2019

andyross left a comment

andyross Oct 23, 2019

andrewboie Oct 23, 2019

stephanosio commented Oct 23, 2019

andrewboie left a comment

andrewboie Oct 23, 2019

andrewboie Oct 23, 2019

stephanosio Oct 23, 2019

andrewboie Oct 23, 2019

stephanosio Oct 23, 2019 •

edited

Loading

stephanosio Oct 23, 2019

stephanosio Oct 23, 2019

andrewboie Oct 23, 2019

stephanosio Oct 23, 2019 •

edited

Loading

andrewboie Oct 23, 2019

stephanosio Oct 23, 2019 •

edited

Loading

andrewboie Oct 23, 2019

stephanosio Oct 23, 2019

andrewboie Oct 23, 2019

stephanosio Oct 23, 2019

stephanosio commented Oct 24, 2019

stephanosio commented Oct 25, 2019 •

edited

Loading

	#ifdef CONFIG_USE_SWITCH
	/* This is a arch function traditionally, but when the switch-based
	* z_swap() is in use it's a simple inline provided by the kernel.
	*/
	static ALWAYS_INLINE void
	z_arch_thread_return_value_set(struct k_thread *thread, unsigned int value)
	{
	thread->swap_retval = value;
	}
	#endif

	#if defined(CONFIG_USE_SWITCH)
	/* When using __switch() a few previously arch-specific items
	* become part of the core OS
	*/

	/** z_swap() return value */
	int swap_retval;

	/** Context handle returned via z_arch_switch() */
	void *switch_handle;
	#endif

	old_thread->swap_retval = -EAGAIN;

	#ifdef CONFIG_SMP
	_current_cpu->swap_ok = 0;

	new_thread->base.cpu = z_arch_curr_cpu()->id;

	if (!is_spinlock) {
	z_smp_release_global_lock(new_thread);
	}
	#endif
	_current = new_thread;
	z_arch_switch(new_thread->switch_handle,
	&old_thread->switch_handle);
	}

	sys_trace_thread_switched_in();

	if (is_spinlock) {
	z_arch_irq_unlock(key);
	} else {
	irq_unlock(key);
	}

	return _current->swap_retval;

[PRELIMINARY] headers: Refactor kernel_structs.h and introduce kernel_arch.h. #20047

[PRELIMINARY] headers: Refactor kernel_structs.h and introduce kernel_arch.h. #20047

Conversation

stephanosio commented Oct 23, 2019

stephanosio commented Oct 23, 2019

ioannisg commented Oct 23, 2019

andyross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanosio commented Oct 23, 2019

andrewboie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanosio Oct 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanosio Oct 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanosio Oct 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanosio commented Oct 24, 2019

stephanosio commented Oct 25, 2019 • edited Loading

stephanosio Oct 23, 2019 •

edited

Loading

stephanosio Oct 23, 2019 •

edited

Loading

stephanosio Oct 23, 2019 •

edited

Loading

stephanosio commented Oct 25, 2019 •

edited

Loading