Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add __io_lock() and __io_unlock() functions to lock io functions #67

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add __io_lock() and __io_unlock() functions to lock io functions #67

wants to merge 2 commits into from

Conversation

gwtsivasiva
Copy link

Great implementation of printf. It is useful for our project in FreeRTOS.
We are currently using it, and would like to improve it adding lock and unlock functions at high level in functions like printf, sprintf to avoid collisions.
That is because multiple threads could call printf without having locks above printf.
The solution might be to use to have locks in the function _putchar, but it is very costly in terms of performance and time.
This kind of lock is better since it allows to send a full string before another thread/core messes up with it.

And of course the locks can be defined by users. Depending on architectures and/or OS, they can be mutexex, semaphores or simple locks using TAS/CAS,...

… as printf in order to avoid collision between multiple threads/cores.
@ledvinap
Copy link

@sivasivarajah : the PR should make locking optional (using #define)

And in my experience, you will run in problems with simple locks.

  • printf can't be called outside of thread (in interrupt context), which is problematic with debug prints.
  • output from multiple threads will be mixed (on printf boundary) and it is not alway possible to do all output with single printf call.

@codecov-io
Copy link

Codecov Report

Merging #67 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #67   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines         359    359           
=====================================
  Hits          359    359
Impacted Files Coverage Δ
printf.c 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21eea6c...3455e53. Read the comment docs.

@gwtsivasiva
Copy link
Author

gwtsivasiva commented Dec 19, 2019

@ledvinap Yes you are right. Those locks should be optional, I added flags to enable or not locks PRINTF_ENABLE_LOCK.

About the issues you raised :

  • The locks and the _putchar should handle this problem. The implementation should be user dependant for these cases.
    Lock can check core's ID/ thread ID and in _putchar, two buffers may be available one for IRQ context and another for normal context. Or only have one and the lock will be bypassed, and the behaviour would be the same as if locks did not exist.

  • This point should be considered in putchar function.

In my implementation for FreeRTOS for our chip(RISC-V GAP8), I have a lock function to handle cores(HW) and threads(SW), since we have 9 cores that can print in parallel, and then the _putchar function fill a buffer with characters. The buffer is sized for 128 char(arbitrary), if the size is reached or a new line is encountered then the buffer is flushed.

Doing this guarantees :

  • Single printf call is enough to do all output
  • Output atomicity
  • Low latency

@mpaland
Copy link
Owner

mpaland commented Jan 1, 2020

Thanks a lot for the PR. But actually I don't like it so much.

I'm using printf with a lot of FreeRTOS projects, too.
An IO-lock of _putchar is often not necessary as the underlying driver should be threadsafe.
An IO-lock of printf itself should not be the problem of printf. I'm using printf for a debug log output from different threads, so the log_out() function sets the lock, executes printf() and releases the lock.

IMHO it's better, if you create an inline function like printf_lock(...) which acquires and releases the lock.
What do you think?

@@ -43,12 +43,16 @@ extern "C" {
/**
* Lock the printf to avoid collision when multiple threads/cores try to output strings/characters.
* This function can implement a mutex, semaphore or whatever lock.
*
* \note PRINTF_ENABLE_LOCK should be defined in order to activate lock function.
*/
void __io_lock();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be wrapped in PRINTF_IO_LOCK preprocessor checks?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually was using the format introduced by Marco to enable features.
Here PRINTF_ENABLE_LOCK is a macro to add in Makefile, and if it is defined, then PRINTF_IO_LOCK will be enabled and lock and unlock functions activated.

@gwtsivasiva
Copy link
Author

Hi Marco, my apologies for this late reply.

I'm using printf with a lot of FreeRTOS projects, too.
An IO-lock of _putchar is often not necessary as the underlying driver should be threadsafe.

That is true, a threadsafe driver should be enough.
But then the output will not be atomic. The output will be mixed and scrambled of multiple threads output won't it?

An IO-lock of printf itself should not be the problem of printf. I'm using printf for a debug log output from different threads, so the log_out() function sets the lock, executes printf() and releases the lock.

Yes I agree for debugging, wrapping the lock in a debug function will be simple and easy. But I want to give user the possibility to print from application side.
I can not have a function called my_own_printf that will wrap a lock function and your implementation of printf, then unlock function. I mean I could but then it won't be user for users.

IMHO it's better, if you create an inline function like printf_lock(...) which acquires and releases the lock.
What do you think?

Instead of __io_lock()? If so why not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants