Avoid locks in DiagsError when target is syslog

[c-taylor]

Various thread wide warn/note messages cause excessive lock contention.

I note from:
include/tscore/Diags.h
src/tscore/Diags.cc

trafficserver/src/tscore/Diags.cc

Line 250 in fef47d7

lock();

That note() and other messages have an explicit lock. This is a problem on busy systems when there are:

• Many threads
• Messages that cause all thread to log simultaneously occur.

In many cases, the types of messages that occur simultaneously are due to error or load events. Ironically this is made worse by the current structure. Change here may improve Diag::Log overhead/behaviour also...

Such messages include:

• Log disks full
• NOTE: Failure threshold met failcount

A lock should not be required when we know the target is syslog (possibly other types also).
A few suggestions:

• Move the syslog conditional to the head of the logic
• Apply conditionals to the lock semantics
• Move the locks into the existing conditionals

[bneradt]

Thank you Craig for filing this.

Just a quick note to record this: the proposed log throttling mechanism would be a potential mitigating feature for this issue. The draft PR is here and there is a design email on the dev mailing list:
#7279

[c-taylor]

I completely agree.

There are many examples of thread-wide messages all with the same trigger/lock race.
I suspect a minor re-factor to the lock/write logic and the log suppression PR combined will make ATS behave more predictably during failure scenarios.

[bneradt]

It looks like we decidedly unlock before writing to syslog on all systems except FreeBSD. Just to make sure we're on the same page: @c-taylor can you verify that you seeing these issues on FreeBSD systems?

The unlock for all systems other than FreeBSD was done a while ago here:
25adb7e

The ticket referenced by that commit is here:
https://issues.apache.org/jira/browse/TS-108

We'll have to look into the current status of syslog/syslog_r for FreeBSD to decide how to proceed.

[c-taylor]

Totally agree.

I this case I’m suggesting the:

1. lock()
2. Conditional
3. Unlock() - unless BSD
4. Syslog

Always has superfluous locks when the targets are syslog on linux.

Re-ordering to: (4,1,2,3)
Is likely the easiest way to reduce this overhead, especially on systems where syslog is the primary message target.

[bneradt]

I see, yes. Thank you for the clarification. And to make this explicit, you've shared offline that you are indeed seeing these issues on Linux.

What you propose looks promising. Maybe we should tuck the locks near the various printf calls for stderr, diags.log, etc, so no lock is grabbed if it is not needed. Or guard the whole block with a check that any of those non-syslog targets are specified and only lock then.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. Marking it stale to flag it for further consideration by the community.