Skip to content

drivers/syslog: fix deadlock by reverting part of the changes from b88a8cf39ff1019ad787c4316b22ce29c7daa2dc#6414

Merged
xiaoxiang781216 merged 1 commit into
apache:masterfrom
pkarashchenko:revet_libc_mutex
Jun 13, 2022
Merged

drivers/syslog: fix deadlock by reverting part of the changes from b88a8cf39ff1019ad787c4316b22ce29c7daa2dc#6414
xiaoxiang781216 merged 1 commit into
apache:masterfrom
pkarashchenko:revet_libc_mutex

Conversation

@pkarashchenko

@pkarashchenko pkarashchenko commented Jun 12, 2022

Copy link
Copy Markdown
Contributor

Summary

Revert part of the changes introduced by b88a8cf dues to deadlock in syslog.

Impact

Fix syslog operation

Testing

Pass CI
Tested with custom SAMe70 based board with 2 channels syslog:

  1. syslog to console
  2. syslog to file on SD card.

@pkarashchenko pkarashchenko requested a review from acassis June 13, 2022 08:56
@pkarashchenko pkarashchenko force-pushed the revet_libc_mutex branch 2 times, most recently from 6770bdb to 433d78f Compare June 13, 2022 09:40
@pkarashchenko pkarashchenko changed the title mutex: Revert part of the changes introduced by cea7953a5623bd48ce1188ce7db9017f92dc0504 mutex: Revert part of the changes introduced by b88a8cf39ff1019ad787c4316b22ce29c7daa2dc Jun 13, 2022
@davids5

davids5 commented Jun 13, 2022

Copy link
Copy Markdown
Contributor

@pkarashchenko On looking at this revert and the replacement. At first blush it looks like semaphore based code does exactly what the rmutex added. Would you please enumerate the difference and the root cause of the deadlock?

@pkarashchenko

pkarashchenko commented Jun 13, 2022

Copy link
Copy Markdown
Contributor Author

@davids5 the deadlock is only caused by changes in drivers/syslog/syslog_device.c.
Initially the syslog take semaphore was returning error code if semaphore is already held (see #6320 (comment))
Other changes are related to introduction of cancellation points in kernel for FLAT build

Comment thread drivers/syslog/syslog_device.c
@davids5

davids5 commented Jun 13, 2022

Copy link
Copy Markdown
Contributor

Let's restore things to a working state and then do a single change for the syslog update.

I was going to suggest splitting this into 2 PR. A revert and a fix for syslog is that what you are saying?

It appears to me that some of the sweeping changes on master have destabilized the project. I would revert all the changes on master and work on an upstream branch until the all the issues are addressed and tested. I would also strongly suggest that future changes like these be done on upstream branches and tested before merging.

@pkarashchenko - you are closer to the problem than I am so I leave it up to you make the call on this.

@pkarashchenko

Copy link
Copy Markdown
Contributor Author

Ok. I will split the change into 2 PRs

…8a8cf

Signed-off-by: Petro Karashchenko <petro.karashchenko@gmail.com>
@pkarashchenko pkarashchenko changed the title mutex: Revert part of the changes introduced by b88a8cf39ff1019ad787c4316b22ce29c7daa2dc drivers/syslog: fix deadlock by reverting part of the changes from b88a8cf39ff1019ad787c4316b22ce29c7daa2dc Jun 13, 2022
@pkarashchenko

Copy link
Copy Markdown
Contributor Author

@davids5 done. Anyway I was trying to address another part in #6376
I think we need to revert libc related changes anyway and then think about better solution.
I found out that currently we have even bigger gap since in case of FLAT build the _SEM_ macro interface is used also for dynamic memory allocation, so malloc and free become cancellation points. I do not have a solid solution for that, but think that we should not rock the boat with changes like #6320 introducing even more inconsistency.
IMO we need to revert another part of the changes (that I removed from this PR) and spend some time on a design review so then it will be possible to come up with a solid solution.

@xiaoxiang781216 xiaoxiang781216 merged commit e42e3aa into apache:master Jun 13, 2022
@pkarashchenko pkarashchenko deleted the revet_libc_mutex branch June 13, 2022 16:16
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.

4 participants