piControl: ensure setting I/O processing state is threadsafe

[linosanfilippo-kunbus]

The piControl kernel modules uses a global bool variable “piDev_g.stopIO” which can be set by means of ioctls to steer the input/output value processing: if the variable is set to “false” input and output are processed as usual, while a value of “true” results in changing of input/output values being supressed (i.e. not passed from/to userspace).

The problem is that the variable is not threadsafe. This means that if it is changed by a userspace process the set value may or may not be visible to the processor that is currently processing the input/output (loop). In the worst case this may result in setting of the variable having no effect.

So ensure the data integrity by using the threadsafe bitoperations provided by the kernel. For this turn the boolean value into a unsigned long and define the bit to determine if I/O processing is enabled or not.

By doing this also correct a related comment in the source code.

Signed-off-by: Lino Sanfilippo l.sanfilippo@kunbus.com

[l1k]

Usually the KB_STOP_IO ioctl is issued while a cycle is in progress. You'd have to be really lucky to issue the ioctl right after a cycle has stopped and a new one is about to begin.

As a result, during a single cycle, some modules will be sent the output values in the process image and some modules will be sent zeroes. For some modules, the input values will be written to the process image and for others they won't.

So usually you have an inconsistency within a single cycle.

This pull request doesn't solve that inconsistency. It just ensures thatpiIoThread() immediately sees the changed value. But that's arguably not a tangible improvement.

Au contraire, the memory barriers implied by the atomic bitops impact performance negatively.

For a real improvement, I think the stopIO value for the next cycle should be stored somewhere and piIoThread() should pick that up upon starting a new cycle. The ioctl should not return until that next cycle has begun.

On the other hand one could argue that the values in the process image change so quickly (every few msecs) that some inconsistency doesn't really hurt. I guess that was Mathias` rationale when he added this feature. But then this pull would be unnecessary.

[linosanfilippo-kunbus]

Usually the KB_STOP_IO ioctl is issued while a cycle is in progress. You'd have to be really lucky to issue the ioctl right after a cycle has stopped and a new one is about to begin.

As a result, during a single cycle, some modules will be sent the output values in the process image and some modules will be sent zeroes. For some modules, the input values will be written to the process image and for others they won't.

So usually you have an inconsistency within a single cycle.

This pull request doesn't solve that inconsistency. It just ensures thatpiIoThread() immediately sees the changed value. But that's arguably not a tangible improvement.

Please note that this is not about seeing stopIO in the middle of a cycle and thus having inconsistent data on a process image for this cycle. It is about seeing stopIO at all: We simply cannot set a (global) variable on one processor and then expect the change to be ever perceived on another processor if the variable is accessed without any synchronization primitives. So in the worst case whatever the user sets by the ioctl() may not have any effect at all. This can be easiest fixed by using the bitoperations provided by the kernel, since these functions already take care of all needed synchronisation when writing/reading bits.

[l1k]

Well, when piIoThread() sleeps (e.g. on a lock) or is preempted, a full memory barrier is implied. In other words, it's going to see the changed variable eventually. Sleeping and preemption happen all the time, not least because UART communication at 115200 bps is slow, so the delay for the stopIO change to take effect can be expected to be very short.

[linosanfilippo-kunbus]

When using barriers for SMP its not enough to have a barrier on one side, this is needed on both (changing thread and reading thread). And even if we have those implicit barriers somewhere this works by accident only. Accessing a variable without any synchronization in multiple threads is just wrong.

[linosanfilippo-kunbus]

Concerning the possible performance impacts by using the bitops, I have made some measurements with trace messages before and after checking the stopIO condition: In both cases (with or without bitops) the time span between the messages was barely measurable: in the worst case 3 microseconds, in the best case 0. So if there is a performance loss due to the bit operations is likely in the range of nanoseconds or below...