[PATCH 0/1] Better i2c access latencies in high load situations

[PATCH 0/1] Better i2c access latencies in high load situations

[Mika Kuoppala]

Hi,

If low priority thread is holding the bus lock while more high
priority threads needs the same i2c bus, priority inversion occurs and
access latency can grow quite large. In my setup i have seen as high as
150ms access latencies in some workloads when the actual physical
transaction is finished in less than 5ms.

The following patch (in separate email) fixes the priority
inversion problem described above by converting the i2c bus lock mutex
to rt_mutex. rt_mutex uses priority inheritance: low priority thread
holding the mutex will get a kick if high priority thread is trying
to acquire the lock.

Thanks,
-- Mika

Mika Kuoppala (1):
  i2c: Prevent priority inversion on top of bus lock by converting it
    to rt_mutex

 drivers/i2c/i2c-core.c |   13 +++++++------
 include/linux/i2c.h    |    3 +--
 2 files changed, 8 insertions(+), 8 deletions(-)

[PATCH 1/1] i2c: Prevent priority inversion on top of bus lock

[Mika Kuoppala]

Low priority thread holding the i2c bus mutex could block
more high priority threads to access the bus resulting in
unacceptable latencies. Change the mutex type to
rt_mutex preventing priority inversion.

Tested-by: Peter Ujfalusi <peter.ujfalusi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Mika Kuoppala <mika.kuoppala-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/i2c-core.c |   12 ++++++------
 include/linux/i2c.h    |    3 +--
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 0e45c29..40f645f 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -548,7 +548,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 		goto out_list;
 	}
 
-	mutex_init(&adap->bus_lock);
+	rt_mutex_init(&adap->bus_lock);
 
 	/* Set default timeout to 1 second if not already set */
 	if (adap->timeout == 0)
@@ -1018,12 +1018,12 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 #endif
 
 		if (in_atomic() || irqs_disabled()) {
-			ret = mutex_trylock(&adap->bus_lock);
+			ret = rt_mutex_trylock(&adap->bus_lock);
 			if (!ret)
 				/* I2C activity is ongoing. */
 				return -EAGAIN;
 		} else {
-			mutex_lock_nested(&adap->bus_lock, adap->level);
+			rt_mutex_lock(&adap->bus_lock);
 		}
 
 		/* Retry automatically on arbitration loss */
@@ -1035,7 +1035,7 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 			if (time_after(jiffies, orig_jiffies + adap->timeout))
 				break;
 		}
-		mutex_unlock(&adap->bus_lock);
+		rt_mutex_unlock(&adap->bus_lock);
 
 		return ret;
 	} else {
@@ -1839,7 +1839,7 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags,
 	flags &= I2C_M_TEN | I2C_CLIENT_PEC;
 
 	if (adapter->algo->smbus_xfer) {
-		mutex_lock(&adapter->bus_lock);
+		rt_mutex_lock(&adapter->bus_lock);
 
 		/* Retry automatically on arbitration loss */
 		orig_jiffies = jiffies;
@@ -1853,7 +1853,7 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags,
 				       orig_jiffies + adapter->timeout))
 				break;
 		}
-		mutex_unlock(&adapter->bus_lock);
+		rt_mutex_unlock(&adapter->bus_lock);
 	} else
 		res = i2c_smbus_xfer_emulated(adapter,addr,flags,read_write,
 					      command, protocol, data);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index f4784c0..ddca85d 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -340,8 +340,7 @@ struct i2c_adapter {
 	void *algo_data;
 
 	/* data fields that are valid for all devices	*/
-	u8 level; 			/* nesting level for lockdep */
-	struct mutex bus_lock;
+	struct rt_mutex bus_lock;
 
 	int timeout;			/* in jiffies */
 	int retries;
-- 
1.6.5.rc0

Re: [PATCH 1/1] i2c: Prevent priority inversion on top of bus lock

[Jean Delvare]

On Wed, 16 Sep 2009 14:17:09 +0300, Mika Kuoppala wrote:
> Low priority thread holding the i2c bus mutex could block
> more high priority threads to access the bus resulting in
> unacceptable latencies. Change the mutex type to
> rt_mutex preventing priority inversion.
> 
> Tested-by: Peter Ujfalusi <peter.ujfalusi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Mika Kuoppala <mika.kuoppala-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/i2c/i2c-core.c |   12 ++++++------
>  include/linux/i2c.h    |    3 +--
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 0e45c29..40f645f 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -548,7 +548,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
>  		goto out_list;
>  	}
>  
> -	mutex_init(&adap->bus_lock);
> +	rt_mutex_init(&adap->bus_lock);
>  
>  	/* Set default timeout to 1 second if not already set */
>  	if (adap->timeout == 0)
> @@ -1018,12 +1018,12 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  #endif
>  
>  		if (in_atomic() || irqs_disabled()) {
> -			ret = mutex_trylock(&adap->bus_lock);
> +			ret = rt_mutex_trylock(&adap->bus_lock);
>  			if (!ret)
>  				/* I2C activity is ongoing. */
>  				return -EAGAIN;
>  		} else {
> -			mutex_lock_nested(&adap->bus_lock, adap->level);
> +			rt_mutex_lock(&adap->bus_lock);

Hmm, rt_mutex doesn't handle locking model validation as regular
mutexes have? Looks like a regression to me.

>  		}
>  
>  		/* Retry automatically on arbitration loss */
> @@ -1035,7 +1035,7 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  			if (time_after(jiffies, orig_jiffies + adap->timeout))
>  				break;
>  		}
> -		mutex_unlock(&adap->bus_lock);
> +		rt_mutex_unlock(&adap->bus_lock);
>  
>  		return ret;
>  	} else {
> @@ -1839,7 +1839,7 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags,
>  	flags &= I2C_M_TEN | I2C_CLIENT_PEC;
>  
>  	if (adapter->algo->smbus_xfer) {
> -		mutex_lock(&adapter->bus_lock);
> +		rt_mutex_lock(&adapter->bus_lock);
>  
>  		/* Retry automatically on arbitration loss */
>  		orig_jiffies = jiffies;
> @@ -1853,7 +1853,7 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags,
>  				       orig_jiffies + adapter->timeout))
>  				break;
>  		}
> -		mutex_unlock(&adapter->bus_lock);
> +		rt_mutex_unlock(&adapter->bus_lock);
>  	} else
>  		res = i2c_smbus_xfer_emulated(adapter,addr,flags,read_write,
>  					      command, protocol, data);
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index f4784c0..ddca85d 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -340,8 +340,7 @@ struct i2c_adapter {
>  	void *algo_data;
>  
>  	/* data fields that are valid for all devices	*/
> -	u8 level; 			/* nesting level for lockdep */
> -	struct mutex bus_lock;
> +	struct rt_mutex bus_lock;
>  
>  	int timeout;			/* in jiffies */
>  	int retries;


-- 
Jean Delvare

Re: [PATCH 1/1] i2c: Prevent priority inversion on top of bus lock

[Mika Kuoppala]

Hi,

On Wed, 2009-09-16 at 13:51 +0200, ext Jean Delvare wrote:

> >  		if (in_atomic() || irqs_disabled()) {
> > -			ret = mutex_trylock(&adap->bus_lock);
> > +			ret = rt_mutex_trylock(&adap->bus_lock);
> >  			if (!ret)
> >  				/* I2C activity is ongoing. */
> >  				return -EAGAIN;
> >  		} else {
> > -			mutex_lock_nested(&adap->bus_lock, adap->level);
> > +			rt_mutex_lock(&adap->bus_lock);
> 
> Hmm, rt_mutex doesn't handle locking model validation as regular
> mutexes have? Looks like a regression to me.
> 

There seems to be no rtmutex_lock_nested in include/linux/rtmutex.h.

CONFIG_DEBUG_RT_MUTEXES help says this:
  This allows rt mutex semantics violations and rt mutex related
  deadlocks (lockups) to be detected and reported automatically.

I don't know if this includes the same stuff as lockdep does.

But as the context is stripped away (adap-level) it's safe to assume
that atleast some functionality is also stripped. Thus it is a
regression.

-- Mika

Re: [PATCH 1/1] i2c: Prevent priority inversion on top of bus lock

[Jean Delvare]

On Wed, 16 Sep 2009 15:35:24 +0300, Mika Kuoppala wrote:
> Hi,
> 
> On Wed, 2009-09-16 at 13:51 +0200, ext Jean Delvare wrote:
> 
> > >  		if (in_atomic() || irqs_disabled()) {
> > > -			ret = mutex_trylock(&adap->bus_lock);
> > > +			ret = rt_mutex_trylock(&adap->bus_lock);
> > >  			if (!ret)
> > >  				/* I2C activity is ongoing. */
> > >  				return -EAGAIN;
> > >  		} else {
> > > -			mutex_lock_nested(&adap->bus_lock, adap->level);
> > > +			rt_mutex_lock(&adap->bus_lock);
> > 
> > Hmm, rt_mutex doesn't handle locking model validation as regular
> > mutexes have? Looks like a regression to me.
> > 
> 
> There seems to be no rtmutex_lock_nested in include/linux/rtmutex.h.
> 
> CONFIG_DEBUG_RT_MUTEXES help says this:
>   This allows rt mutex semantics violations and rt mutex related
>   deadlocks (lockups) to be detected and reported automatically.
> 
> I don't know if this includes the same stuff as lockdep does.
> 
> But as the context is stripped away (adap-level) it's safe to assume
> that atleast some functionality is also stripped. Thus it is a
> regression.

OK. This is the only place where the adapter's nesting level is used,
so if we apply your patch, we might as well get rid of it.

-- 
Jean Delvare

Re: [PATCH 0/1] Better i2c access latencies in high load situations

[Jean Delvare]

Hi Mika,

On Wed, 16 Sep 2009 14:17:08 +0300, Mika Kuoppala wrote:
> If low priority thread is holding the bus lock while more high
> priority threads needs the same i2c bus, priority inversion occurs and
> access latency can grow quite large. In my setup i have seen as high as
> 150ms access latencies in some workloads when the actual physical
> transaction is finished in less than 5ms.
> 
> The following patch (in separate email) fixes the priority
> inversion problem described above by converting the i2c bus lock mutex
> to rt_mutex. rt_mutex uses priority inheritance: low priority thread
> holding the mutex will get a kick if high priority thread is trying
> to acquire the lock.

Can you please define "get a kick"? I don't know anything about
rt_mutex.

> 
> Thanks,
> -- Mika
> 
> Mika Kuoppala (1):
>   i2c: Prevent priority inversion on top of bus lock by converting it
>     to rt_mutex
> 
>  drivers/i2c/i2c-core.c |   13 +++++++------
>  include/linux/i2c.h    |    3 +--
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 


-- 
Jean Delvare

Re: [PATCH 0/1] Better i2c access latencies in high load situations

[Mika Kuoppala]

Hi Jean,

On Wed, 2009-09-16 at 13:49 +0200, ext Jean Delvare wrote:

> Can you please define "get a kick"? I don't know anything about
> rt_mutex.
> 

Sorry for using a vague metaphor. Documentation/rt-mutex.txt explains it
as:

"A low priority owner of a rt-mutex inherits the priority of a higher
priority waiter until the rt-mutex is released. If the temporarily
boosted owner blocks on a rt-mutex itself it propagates the priority
boosting to the owner of the other rt_mutex it gets blocked on. The
priority boosting is immediately removed once the rt_mutex has been
unlocked."

You might want to also take a look at Documentation/rt-mutex-design.txt

--Mika

Re: [PATCH 0/1] Better i2c access latencies in high load situations

[Jean Delvare]

On Wed, 16 Sep 2009 15:08:55 +0300, Mika Kuoppala wrote:
> Hi Jean,
> 
> On Wed, 2009-09-16 at 13:49 +0200, ext Jean Delvare wrote:
> 
> > Can you please define "get a kick"? I don't know anything about
> > rt_mutex.
> > 
> 
> Sorry for using a vague metaphor. Documentation/rt-mutex.txt explains it
> as:
> 
> "A low priority owner of a rt-mutex inherits the priority of a higher
> priority waiter until the rt-mutex is released. If the temporarily
> boosted owner blocks on a rt-mutex itself it propagates the priority
> boosting to the owner of the other rt_mutex it gets blocked on. The
> priority boosting is immediately removed once the rt_mutex has been
> unlocked."
> 
> You might want to also take a look at Documentation/rt-mutex-design.txt

Thanks for the clarification. It all makes a lot of sense. I'll give
your patch a try, although I don't use I2C for anything time-critical
so I doubt it makes a difference for me.

But now I am curious, why don't we use rt_mutex instead of regular
mutex all around the place?

-- 
Jean Delvare

Re: [PATCH 0/1] Better i2c access latencies in high load situations

[Mika Kuoppala]

Hi!

On Wed, 2009-09-16 at 22:43 +0200, ext Jean Delvare wrote:
> On Wed, 16 Sep 2009 15:08:55 +0300, Mika Kuoppala wrote:
> > Hi Jean,
> > 
> > On Wed, 2009-09-16 at 13:49 +0200, ext Jean Delvare wrote:
> > 
> > > Can you please define "get a kick"? I don't know anything about
> > > rt_mutex.
> > > 
> > 
> > Sorry for using a vague metaphor. Documentation/rt-mutex.txt explains it
> > as:
> > 
> > "A low priority owner of a rt-mutex inherits the priority of a higher
> > priority waiter until the rt-mutex is released. If the temporarily
> > boosted owner blocks on a rt-mutex itself it propagates the priority
> > boosting to the owner of the other rt_mutex it gets blocked on. The
> > priority boosting is immediately removed once the rt_mutex has been
> > unlocked."
> > 
> > You might want to also take a look at Documentation/rt-mutex-design.txt
> 
> Thanks for the clarification. It all makes a lot of sense. I'll give
> your patch a try, although I don't use I2C for anything time-critical
> so I doubt it makes a difference for me.
> 

I tried to write an application which could show the priority inversion
in action on top of this bus mutex. This application spawns 3 different
child processes each reading one byte of data from a specified slave
address and register. Multiple times. Each child has different priority.
The program to produce these results is at the end of this email.

Without patch: i2c: Prevent priority inversion on top of bus lock
~ # ./a.out
Spawning 3 workers with different priorities
Each worker will read one byte from /dev/i2c-2:0x4b:0x00 10000 times
( 941)PRIO -5     rt  7973 ms lat:  579 min, 313294 max,  795 avg (us)
( 940)PRIO 0      rt 16412 ms lat:  549 min, 796479 max, 1637 avg (us)
( 942)SCHED_FIFO  rt 28148 ms lat:  580 min, 796509 max, 1535 avg (us)
Total run time 28152313 usecs

With patch: i2c: Prevent priority inversion on top of bus lock
~ # ./a.out
Spawning 3 workers with different priorities
Each worker will read one byte from /dev/i2c-2:0x4b:0x00 10000 times
( 960)PRIO -5     rt 13069 ms lat:  580 min,   2472 max, 1302 avg (us)
( 959)PRIO 0      rt 20420 ms lat:  519 min,  16632 max, 2037 avg (us)
( 961)SCHED_FIFO  rt 20420 ms lat:  580 min,   1282 max,  762 avg (us)
Total run time 20424682 usecs

PRIO -5 and PRIO 0 process are busylooping on top of i2c_read and the
SCHED_FIFO is sleeping 1ms after each read for not to steal all cpu
cycles.

As we can see from the results without the patch, the SCHED_FIFO doesn't
have much effect and maximum latencies grow quite large. 

With patch, the maximum latencies are much better. Averages follow the
respective priorities. rt_mutex ensures that the wait time to reaquire
the lock is kept minimum by not letting low priority process to hold it.
This leads to better bus utilization and we finish almost 8 seconds
(~27%) quicker in total. And most importantly the lower priority
processes can't keep the bus mutex for themselves and destroy the
SCHED_FIFO access latencies.

Obviously this benchmark is made to emphasize this problem. Nevertheless
myself and Peter have witnessed this problem in real world workload.

> But now I am curious, why don't we use rt_mutex instead of regular
> mutex all around the place?
> 

What do you mean by all around the place ?

In scope of i2c-core there is no point of converting the client list
lock into rt_mutex due to lack of contention. As there is no free lunch
the rt_mutex struct takes more space and cpu cycles. Also it could be
that rt_mutex is more novel feature and code is converted conservatively
and only when there is real need for it.

I have attached the test program in this mail. You need to change the
slave address and possibly the register to suit your setup. It would be
very interesting to see the numbers from some completely different hw
setup =)

Thanks,
-- Mika

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/ioctl.h>
#include <sys/time.h>
#include <sys/resource.h>
#include <sys/wait.h>
#include <fcntl.h>
#include <sched.h>
#include <linux/i2c-dev.h>

#define READS_PER_CHILD 10000

const char * const i2c_bus_name = "/dev/i2c-2";
const unsigned char i2c_addr = 0x4b;
const unsigned char i2c_reg = 0x00;

struct lat_stats {
	unsigned long min;
	unsigned long max;
	unsigned long long total;
	unsigned long count;
};

static struct lat_stats stats = { 0, 0, 0, 0 };

static void update_lat_stats(unsigned long usecs)
{
	if (stats.count == 0) {
		stats.min = usecs;
		stats.max = usecs;
	} else {
		if (stats.min > usecs)
			stats.min = usecs;
		else if(stats.max < usecs)
			stats.max = usecs;
	}

	stats.total += usecs;
	stats.count++;
}

static void print_lat_stats(const char *s, struct timeval *start, struct
timeval *end)
{
	unsigned long usecs;

	usecs = (end->tv_sec - start->tv_sec) * 1000 * 1000;
	usecs += (end->tv_usec - start->tv_usec);

	printf("(%4d)%-11s rt %5lu ms ", getpid(), s, usecs / 1000);
	printf("lat: %4lu min, %6lu max, %4llu avg (us)\n",
	       stats.min, stats.max, stats.total / stats.count);
}

static int read_timed(int fd, int reg)
{
	int r;
	struct timeval start, end;
	unsigned long usecs;

	gettimeofday(&start, NULL);
	r = i2c_smbus_read_byte_data(fd, reg);
	gettimeofday(&end, NULL);
	if (r < 0) {
		printf("Error reading: %s\n", strerror(errno));
		return r;
	}

	usecs = (end.tv_sec - start.tv_sec) * 1000 * 1000;
	usecs += (end.tv_usec - start.tv_usec);

	update_lat_stats(usecs);

	return r;
}

static int child1_work(int fd)
{
	int r;
	struct timeval start, end;

	printf("pid: %d is PRIO_PROCESS 0\n", getpid());
	r = setpriority(PRIO_PROCESS, 0, 0);

	gettimeofday(&start, NULL);
	while (1) {
		r = read_timed(fd, i2c_reg);
		if (r < 0) {
			printf("Error reading: %s\n", strerror(errno));
			return r;
		}

		if(stats.count >= READS_PER_CHILD)
			break;
	}
	gettimeofday(&end, NULL);

	print_lat_stats("PRIO 0", &start, &end);

	return 0;
}

static int child2_work(int fd)
{
	int r;
	struct timeval start, end;

	printf("pid: %d is PRIO_PROCESS -5\n", getpid());
	r = setpriority(PRIO_PROCESS, 0, -5);

	gettimeofday(&start, NULL);
	while (1) {
		r = read_timed(fd, i2c_reg);
		if (r < 0) {
			printf("Error reading: %s\n", strerror(errno));
			return r;
		}

		if(stats.count >= READS_PER_CHILD)
			break;
	}
	gettimeofday(&end, NULL);

	print_lat_stats("PRIO -5", &start, &end);

	return 0;
}

static int child3_work(int fd)
{
	int r;
	struct timeval start, end;
	struct sched_param sparam = { 0 };

	sparam.sched_priority = 99;

	printf("pid: %d is SCHED_FIFO\n", getpid());
	r = sched_setscheduler(0, SCHED_FIFO, &sparam);
	if (r == -1) {
		printf("Error setting scheduler: %s\n", strerror(errno));
		return r;
	}

	gettimeofday(&start, NULL);
	while (1) {
		r = read_timed(fd, i2c_reg);
		if (r < 0) {
			printf("Error reading: %s\n", strerror(errno));
			return r;
		}

		if(stats.count >= READS_PER_CHILD)
			break;

		/* Give others a chance to do some work */
		usleep(1000);
	}
	gettimeofday(&end, NULL);

	print_lat_stats("SCHED_FIFO", &start, &end);

	return 0;
}

static int fork_work(int (*f)(int fd), int fd)
{
	int r;

	r = fork();
	if (r == 0) {
		r = f(fd);
		if (r != 0)
			printf("Child %d returned with code %d\n", getpid(), r);
		exit(r);
	} else if (r < 0) {
		printf("Error in forking: %s\n", strerror(errno));
		exit(-1);
	}

	return r;
}


int main(int argc, char *argv[])
{
	int fd = -1;
	int r = -1;
	int pid_work1 = 0;
	int pid_work2 = 0;
	int pid_work3 = 0;
	struct timeval start, end;
	unsigned long usecs;


	fd = open(i2c_bus_name, O_RDWR);
	if (fd == -1)
		perror("error opening: ");

	r = ioctl(fd, I2C_SLAVE_FORCE, i2c_addr);
	if (r < 0) {
		printf("Error in I2C_SLAVE_FORCE(0x%02x) ioctl failed: %s\n",
		       i2c_addr, strerror(errno));
		goto out;
	}

	printf("Spawning 3 workers with different priorities\n");
	printf("Each worker will read one byte from %s:0x%02x:0x%02x %d times
\n",
	       i2c_bus_name, i2c_addr, i2c_reg, READS_PER_CHILD);

	gettimeofday(&start, NULL);

	pid_work1 = fork_work(child1_work, fd);
	pid_work2 = fork_work(child2_work, fd);
	pid_work3 = fork_work(child3_work, fd);

	waitpid(pid_work1, NULL, 0);
	waitpid(pid_work2, NULL, 0);
	waitpid(pid_work3, NULL, 0);

	gettimeofday(&end, NULL);

	usecs = (end.tv_sec - start.tv_sec) * 1000 * 1000;
	usecs += (end.tv_usec - start.tv_usec);

	printf("Total run time %lu usecs\n", usecs);

	r = 0;
out:
	if (fd != -1)
		close(fd);

	return r;
}

Re: [PATCH 0/1] Better i2c access latencies in high load situations

[Jean Delvare]

Hi Mika,

On Mon, 21 Sep 2009 16:14:07 +0300, Mika Kuoppala wrote:
> On Wed, 2009-09-16 at 22:43 +0200, ext Jean Delvare wrote:
> > On Wed, 16 Sep 2009 15:08:55 +0300, Mika Kuoppala wrote:
> > > Hi Jean,
> > > 
> > > On Wed, 2009-09-16 at 13:49 +0200, ext Jean Delvare wrote:
> > > 
> > > > Can you please define "get a kick"? I don't know anything about
> > > > rt_mutex.
> > > > 
> > > 
> > > Sorry for using a vague metaphor. Documentation/rt-mutex.txt explains it
> > > as:
> > > 
> > > "A low priority owner of a rt-mutex inherits the priority of a higher
> > > priority waiter until the rt-mutex is released. If the temporarily
> > > boosted owner blocks on a rt-mutex itself it propagates the priority
> > > boosting to the owner of the other rt_mutex it gets blocked on. The
> > > priority boosting is immediately removed once the rt_mutex has been
> > > unlocked."
> > > 
> > > You might want to also take a look at Documentation/rt-mutex-design.txt
> > 
> > Thanks for the clarification. It all makes a lot of sense. I'll give
> > your patch a try, although I don't use I2C for anything time-critical
> > so I doubt it makes a difference for me.
> > 
> 
> I tried to write an application which could show the priority inversion
> in action on top of this bus mutex. This application spawns 3 different
> child processes each reading one byte of data from a specified slave
> address and register. Multiple times. Each child has different priority.
> The program to produce these results is at the end of this email.
> 
> Without patch: i2c: Prevent priority inversion on top of bus lock
> ~ # ./a.out
> Spawning 3 workers with different priorities
> Each worker will read one byte from /dev/i2c-2:0x4b:0x00 10000 times
> ( 941)PRIO -5     rt  7973 ms lat:  579 min, 313294 max,  795 avg (us)
> ( 940)PRIO 0      rt 16412 ms lat:  549 min, 796479 max, 1637 avg (us)
> ( 942)SCHED_FIFO  rt 28148 ms lat:  580 min, 796509 max, 1535 avg (us)
> Total run time 28152313 usecs
> 
> With patch: i2c: Prevent priority inversion on top of bus lock
> ~ # ./a.out
> Spawning 3 workers with different priorities
> Each worker will read one byte from /dev/i2c-2:0x4b:0x00 10000 times
> ( 960)PRIO -5     rt 13069 ms lat:  580 min,   2472 max, 1302 avg (us)
> ( 959)PRIO 0      rt 20420 ms lat:  519 min,  16632 max, 2037 avg (us)
> ( 961)SCHED_FIFO  rt 20420 ms lat:  580 min,   1282 max,  762 avg (us)
> Total run time 20424682 usecs
> 
> PRIO -5 and PRIO 0 process are busylooping on top of i2c_read and the
> SCHED_FIFO is sleeping 1ms after each read for not to steal all cpu
> cycles.
> 
> As we can see from the results without the patch, the SCHED_FIFO doesn't
> have much effect and maximum latencies grow quite large.

Indeed. I see similar results here (kernel 2.6.31, i2c-nforce2):

Before patch:

# ./prioinv 
Spawning 3 workers with different priorities
Each worker will read one byte from /dev/i2c-0:0x51:0x00 1000 times
pid: 4583 is PRIO_PROCESS 0
pid: 4584 is PRIO_PROCESS -5
pid: 4585 is SCHED_FIFO
(4583)PRIO 0      rt 19308 ms lat: 4974 min,  24989 max, 19308 avg (us)
(4584)PRIO -5     rt 23988 ms lat: 15912 min, 836616 max, 23987 avg (us)
(4585)SCHED_FIFO  rt 23996 ms lat: 14909 min, 844525 max, 22990 avg (us)
Total run time 23997362 usecs

With patch:

# ./prioinv 
Spawning 3 workers with different priorities
Each worker will read one byte from /dev/i2c-0:0x51:0x00 1000 times
pid: 2021 is PRIO_PROCESS -5
pid: 2022 is SCHED_FIFO
pid: 2020 is PRIO_PROCESS 0
(2022)SCHED_FIFO  rt 15948 ms lat: 6817 min,  15171 max, 14941 avg (us)
(2021)PRIO -5     rt 15997 ms lat: 5029 min,  32173 max, 15996 avg (us)
(2020)PRIO 0      rt 24012 ms lat: 4321 min, 16004516 max, 24011 avg (us)
Total run time 24013759 usecs

> With patch, the maximum latencies are much better. Averages follow the
> respective priorities. rt_mutex ensures that the wait time to reaquire
> the lock is kept minimum by not letting low priority process to hold it.
> This leads to better bus utilization and we finish almost 8 seconds
> (~27%) quicker in total. And most importantly the lower priority
> processes can't keep the bus mutex for themselves and destroy the
> SCHED_FIFO access latencies.

I don't see the general performance improvement you see, but the
latency improvement is there.

> Obviously this benchmark is made to emphasize this problem. Nevertheless
> myself and Peter have witnessed this problem in real world workload.
> 
> > But now I am curious, why don't we use rt_mutex instead of regular
> > mutex all around the place?
> > 
> 
> What do you mean by all around the place ?

I meant all the kernel mutexes; no kidding :)

> In scope of i2c-core there is no point of converting the client list
> lock into rt_mutex due to lack of contention. As there is no free lunch
> the rt_mutex struct takes more space and cpu cycles. Also it could be
> that rt_mutex is more novel feature and code is converted conservatively
> and only when there is real need for it.

Ah, OK, thanks for the explanation. Now I know enough to consider
applying your patch. We're unfortunately a little late for 2.6.32, so
my plan is to include your patch in my i2c tree and schedule it for
inclusion in kernel 2.6.33.

Note that I had to modify drivers/net/sfc/sfe4001.c too as it accesses
the mutex directly.

-- 
Jean Delvare