libc: newlib: make sbrk() thread-safe

[andrewboie]

Concurrent use of this function could lead to corruption.
Add a semaphore to synchronize this.

Signed-off-by: Andrew Boie [email protected]

[andrewboie]

CI failure is broken in master branch, I'm opening a bug

[andyross]

Maybe a spinlock (which becomes an irq_lock on uniprocessor systems) would be a better fit here? This is a fast operation with bounded runtime. There's no real benefit to trying to block a process on it and significant extra code involved in sucking in the mutex implementation.

[andrewboie]

This code has to run in user mode, so a spinlock is not appropriate.

[andyross]

Futex then? :)

Just seems like the (comparatively very heavyweight, especially when done via syscalls) blocking mechanism is exactly what we don't want when synchronizing a tiny bit of deterministic code in the guts of the allocator.

[andrewboie]

@andyross that's what sys_mutex is.
Implementation isn't completely done to use atomic ops, but that is what it is for. See #15138

[andyross]

I thought sys_mutex was the priority inheriting thing (which we don't use much from elsewhere in the kernel, will suck in non-trivial extra code for all newlib targets, and which is known to have SMP bugs)?

You win.

[andrewboie]

I thought sys_mutex was the priority inheriting thing

It is. It is backed by a k_mutex on the kernel side. The design of sys mutex is to have the same semantics as FUTEX_LOCK_PI / FUTEX_UNLOCK_PI

which is known to have SMP bugs

Which bugs are these?? We should label them high priority and fix them...

will suck in non-trivial extra code for all newlib targets

What do you want here?
This code in its current state is broken.
Should I use a sys_sem instead?

[andrewboie]

Discussed some more offline, going to use a one-count sys_sem instead of a sys_mutex. Need to wait until the sys_sem patch is merged #16523

[zephyrbot]

Found the following issues, please fix and resubmit:

checkpatch issues

-:34: ERROR:MULTISTATEMENT_MACRO_USE_DO_WHILE: Macros with multiple statements should be enclosed in a do - while loop
#34: FILE: include/sys/sem.h:67:
+#define SYS_SEM_DEFINE(_name, _initial_count, _count_limit) \
+	Z_DECL_ALIGN(struct sys_sem) _name \
+		__in_section(_k_sem, static, _name) = { \
+		.kernel_sem = Z_SEM_INITIALIZER(_name.kernel_sem, \
+						_initial_count, _count_limit) \
+	}; \
+	BUILD_ASSERT(((_count_limit) != 0) && \
+		     ((_initial_count) <= (_count_limit)))

- total: 1 errors, 0 warnings, 104 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

[andrewboie]

Changed to a sys_sem instead of a mutex.
Added a patch to add static sys_sem initializers.

[andrewboie]

-:29: ERROR:MULTISTATEMENT_MACRO_USE_DO_WHILE: Macros with multiple statements should be enclosed in a do - while loop

This can't be done at toplevel.
K_SEM_DEFINE() does the same thing.
Spurious error!

addressed

[kaidoho]

@andrewboie may I ask why you haven't used the malloc_lock / malloc_unlock hooks defined within newlib which guard the code around newlib's calls to sbrk?

Perhaps a separate issue but closely related to this as malloc_lock / unlock require a reentrancy struct. The documentation within newlib's reent.h instructs us to either provide reentrant versions of all syscalls and manage reentrancy structures ourself, or to keep a REENT_ struct for each thread and provide newlib access to these structs via the __getreent() hook. As I can find neither an implementation of the *_r nor __getreent() - did I go terribly wrong somewhere or is the integration of newlib not completed yet?

[andrewboie]

@andrewboie may I ask why you haven't used the malloc_lock / malloc_unlock hooks defined within newlib which guard the code around newlib's calls to sbrk?

I didn't know they existed.

Perhaps a separate issue but closely related to this as malloc_lock / unlock require a reentrancy struct. The documentation within newlib's reent.h instructs us to either provide reentrant versions of all syscalls and manage reentrancy structures ourself, or to keep a REENT_ struct for each thread and provide newlib access to these structs via the __getreent() hook. As I can find neither an implementation of the *_r nor __getreent() - did I go terribly wrong somewhere or is the integration of newlib not completed yet?

@kaidoho Zephyr's newlib integration predates my involvement with the project -- I'm not the original author. I fixed the concurrency issue with sbrk() in a way I knew how, but if there's a better way to do some things please file a GH issue (or send patches)

[kaidoho]

Ok, will file an GH issue. Thx for your response @andrewboie