Re: [PATCH] irqchip/gic-v3-its: handle rd_idx wrapping in its_wait_for_range_completion()

Re: [PATCH] irqchip/gic-v3-its: handle rd_idx wrapping in its_wait_for_range_completion()

[Marc Zyngier]

On Sun, 11 Feb 2018 03:42:01 +0000,
Yang Yingliang wrote:

Hi Yang,

> In direct case, rd_idx will wrap if other cpus post commands
> that make rd_idx increase. When rd_idx wrapped, the driver prints
> timeout messages but in fact the command is finished. To handle
> this case by adding last_rd_id to check rd_idx whether wrapped.
> 
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>

Please always Cc LKML on irqchip related patches.

> ---
>  drivers/irqchip/irq-gic-v3-its.c | 84 ++++++++++++++++++++++------------------
>  1 file changed, 46 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 06f025f..d7176d1 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -713,7 +713,8 @@ static void its_flush_cmd(struct its_node *its, struct its_cmd_block *cmd)
>  
>  static int its_wait_for_range_completion(struct its_node *its,
>  					 struct its_cmd_block *from,
> -					 struct its_cmd_block *to)
> +					 struct its_cmd_block *to,
> +					 u64 last_rd_idx)
>  {
>  	u64 rd_idx, from_idx, to_idx;
>  	u32 count = 1000000;	/* 1s! */
> @@ -724,9 +725,14 @@ static int its_wait_for_range_completion(struct its_node *its,
>  	while (1) {
>  		rd_idx = readl_relaxed(its->base + GITS_CREADR);
>  
> -		/* Direct case */
> -		if (from_idx < to_idx && rd_idx >= to_idx)
> -			break;
> +
> +		/*
> +		 * Direct case. In this case, rd_idx may wrapped,
> +		 * because other cpus may post commands that make
> +		 * rd_idx increase.
> +		 */
> +		if (from_idx < to_idx && (rd_idx >= to_idx || rd_idx < last_rd_idx))
> +				break;
>  
>  		/* Wrapped case */
>  		if (from_idx >= to_idx && rd_idx >= to_idx && rd_idx < from_idx)
> @@ -746,40 +752,42 @@ static int its_wait_for_range_completion(struct its_node *its,

[...]

> +	last_rd_idx = readl_relaxed(its->base + GITS_CREADR);			\

What is this last_rd_idx exactly? It is just some random sampling of
the read pointer after we've posted our commands. It can still be in
any position. And if the reader as wrapped because other CPUs are
feeding more commands to the queue, it could just as well overtake
last_rd_idx, making your new condition just as false. Yes, this is
unlikely, but still.

If you're going to harden the command queue wrapping, I'd suggest you
implement a generation mechanism so that we can easily work out if
we've seen the queue wrapping or not. THis would lift any kind of
ambiguity once and for all.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

Re: [PATCH] irqchip/gic-v3-its: handle rd_idx wrapping in its_wait_for_range_completion()

[Yang Yingliang]

On 2018/2/12 2:45, Marc Zyngier wrote:
Hi, Marc

Sorry for replying so late.
> On Sun, 11 Feb 2018 03:42:01 +0000,
> Yang Yingliang wrote:
>
> Hi Yang,
>
>> In direct case, rd_idx will wrap if other cpus post commands
>> that make rd_idx increase. When rd_idx wrapped, the driver prints
>> timeout messages but in fact the command is finished. To handle
>> this case by adding last_rd_id to check rd_idx whether wrapped.
>>
>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> Please always Cc LKML on irqchip related patches.
>
>> ---
>>   drivers/irqchip/irq-gic-v3-its.c | 84 ++++++++++++++++++++++------------------
>>   1 file changed, 46 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index 06f025f..d7176d1 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -713,7 +713,8 @@ static void its_flush_cmd(struct its_node *its, struct its_cmd_block *cmd)
>>   
>>   static int its_wait_for_range_completion(struct its_node *its,
>>   					 struct its_cmd_block *from,
>> -					 struct its_cmd_block *to)
>> +					 struct its_cmd_block *to,
>> +					 u64 last_rd_idx)
>>   {
>>   	u64 rd_idx, from_idx, to_idx;
>>   	u32 count = 1000000;	/* 1s! */
>> @@ -724,9 +725,14 @@ static int its_wait_for_range_completion(struct its_node *its,
>>   	while (1) {
>>   		rd_idx = readl_relaxed(its->base + GITS_CREADR);
>>   
>> -		/* Direct case */
>> -		if (from_idx < to_idx && rd_idx >= to_idx)
>> -			break;
>> +
>> +		/*
>> +		 * Direct case. In this case, rd_idx may wrapped,
>> +		 * because other cpus may post commands that make
>> +		 * rd_idx increase.
>> +		 */
>> +		if (from_idx < to_idx && (rd_idx >= to_idx || rd_idx < last_rd_idx))
>> +				break;
>>   
>>   		/* Wrapped case */
>>   		if (from_idx >= to_idx && rd_idx >= to_idx && rd_idx < from_idx)
>> @@ -746,40 +752,42 @@ static int its_wait_for_range_completion(struct its_node *its,
> [...]
>
>> +	last_rd_idx = readl_relaxed(its->base + GITS_CREADR);			\
> What is this last_rd_idx exactly? It is just some random sampling of
> the read pointer after we've posted our commands. It can still be in
> any position. And if the reader as wrapped because other CPUs are
> feeding more commands to the queue, it could just as well overtake
> last_rd_idx, making your new condition just as false. Yes, this is
> unlikely, but still.
>
> If you're going to harden the command queue wrapping, I'd suggest you
> implement a generation mechanism so that we can easily work out if
> we've seen the queue wrapping or not. THis would lift any kind of
> ambiguity once and for all.
I get a lot of timeout messages, when I am doing set affinity stress 
testing which
will post many its commands. I will try another way to handle the 
wrapped case.

Regards,
Yang
>
> Thanks,
>
> 	M.
>

[RFC PATCH] irqchip/gic-v3-its: handle wrapped case in its_wait_for_range_completion()

[Yang Yingliang]

From: Yang Yingliang <yangyingliang@huawei.com>

While cpus posting a bunch of ITS commands, the cmd_queue and rd_idx will
be wrapped easily. And current way of handling wrapped case is not quite
right.
Such as, in direct case, rd_idx will wrap if other cpus post commands
that make rd_idx increase. When rd_idx wrapped, the driver prints
timeout messages but in fact the command is finished.

This patch adds two variables to count wrapped times of ITS commands and
read index. With these two variables, the driver can handle wrapped case
correctly.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
  drivers/irqchip/irq-gic-v3-its.c | 72 
+++++++++++++++++++++++++++++++++-------
  1 file changed, 60 insertions(+), 12 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c 
b/drivers/irqchip/irq-gic-v3-its.c
index 1d3056f..a03e18e 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -111,6 +111,9 @@ struct its_node {
  	u32			pre_its_base; /* for Socionext Synquacer */
  	bool			is_v4;
  	int			vlpi_redist_offset;
+	int 			last_rd;
+	u64			cmd_wrapped_cnt;
+	u64			rd_wrapped_cnt;
  };

  #define ITS_ITT_ALIGN		SZ_256
@@ -662,6 +665,7 @@ static int its_queue_full(struct its_node *its)

  static struct its_cmd_block *its_allocate_entry(struct its_node *its)
  {
+	u32 rd;
  	struct its_cmd_block *cmd;
  	u32 count = 1000000;	/* 1s! */

@@ -675,11 +679,24 @@ static struct its_cmd_block 
*its_allocate_entry(struct its_node *its)
  		udelay(1);
  	}

+	/*
+	 * Here is protected by its->lock and driver cannot allocate
+	 * ITS commands, if ITS command queue is full, so the read
+	 * won't wrap twice between this rd_idx and last rd_idx.
+	 * Count rd wrapped times here is safe.
+	 */
+	rd = readl_relaxed(its->base + GITS_CREADR);
+	if (rd < its->last_rd)
+		its->rd_wrapped_cnt++;
+	its->last_rd = rd;
+
  	cmd = its->cmd_write++;

  	/* Handle queue wrapping */
-	if (its->cmd_write == (its->cmd_base + ITS_CMD_QUEUE_NR_ENTRIES))
+	if (its->cmd_write == (its->cmd_base + ITS_CMD_QUEUE_NR_ENTRIES)) {
  		its->cmd_write = its->cmd_base;
+		its->cmd_wrapped_cnt++;
+	}

  	/* Clear command  */
  	cmd->raw_cmd[0] = 0;
@@ -713,29 +730,57 @@ static void its_flush_cmd(struct its_node *its, 
struct its_cmd_block *cmd)

  static int its_wait_for_range_completion(struct its_node *its,
  					 struct its_cmd_block *from,
-					 struct its_cmd_block *to)
+					 struct its_cmd_block *to,
+					 u64 last_cmd_wrapped_cnt)
  {
-	u64 rd_idx, from_idx, to_idx;
+	unsigned long flags;
+	u64 rd_idx, from_idx, to_idx, rd_wrapped_cnt;
  	u32 count = 1000000;	/* 1s! */

  	from_idx = its_cmd_ptr_to_offset(its, from);
  	to_idx = its_cmd_ptr_to_offset(its, to);

  	while (1) {
+		raw_spin_lock_irqsave(&its->lock, flags);
  		rd_idx = readl_relaxed(its->base + GITS_CREADR);
+		if (rd_idx < its->last_rd)
+			its->rd_wrapped_cnt++;
+		its->last_rd = rd_idx;
+		rd_wrapped_cnt = its->rd_wrapped_cnt;
+		raw_spin_unlock_irqrestore(&its->lock, flags);

-		/* Direct case */
-		if (from_idx < to_idx && rd_idx >= to_idx)
-			break;
-
-		/* Wrapped case */
-		if (from_idx >= to_idx && rd_idx >= to_idx && rd_idx < from_idx)
+		/*
+		 * If rd_wrapped_cnt > last_cmd_wrapped_cnt:
+		 * there are a lot of ITS commands posted by
+		 * other cpus and ITS is fast.
+		 *
+		 * If rd_wrapped_cnt < last_cmd_wrapped_cnt:
+		 * ITS is slow, there are some ITS commands
+		 * not finished.
+		 *
+		 * If rd_wrapped_cnt == last_cmd_wrapped_cnt:
+		 * it's common case.
+		 */
+		if (rd_wrapped_cnt > last_cmd_wrapped_cnt) {
+			/*
+			 * There is a lot of ITS commands posted by other cpus,
+			 * it make rd_idx move foward fast and wrap.
+			 */
  			break;
+		} else if (rd_wrapped_cnt == last_cmd_wrapped_cnt) {
+			/* Direct case */
+			if (from_idx < to_idx && rd_idx >= to_idx)
+				break;
+
+			/* Wrapped case */
+			if (from_idx >= to_idx && rd_idx >= to_idx && rd_idx < from_idx)
+				break;
+		}

  		count--;
  		if (!count) {
-			pr_err_ratelimited("ITS queue timeout (%llu %llu %llu)\n",
-					   from_idx, to_idx, rd_idx);
+			pr_err_ratelimited("ITS queue timeout (%llu %llu %llu) %llu %llu\n",
+					   from_idx, to_idx, rd_idx, rd_wrapped_cnt, last_cmd_wrapped_cnt);
  			return -1;
  		}
  		cpu_relax();
@@ -754,6 +799,7 @@ void name(struct its_node *its,						\
  	struct its_cmd_block *cmd, *sync_cmd, *next_cmd;		\
  	synctype *sync_obj;						\
  	unsigned long flags;						\
+	u64 cmd_wrapped_cnt;						\
  									\
  	raw_spin_lock_irqsave(&its->lock, flags);			\
  									\
@@ -776,9 +822,11 @@ void name(struct its_node *its,						\
  									\
  post:									\
  	next_cmd = its_post_commands(its);				\
+	cmd_wrapped_cnt = its->cmd_wrapped_cnt;				\
  	raw_spin_unlock_irqrestore(&its->lock, flags);			\
  									\
-	if (its_wait_for_range_completion(its, cmd, next_cmd))		\
+	if (its_wait_for_range_completion(its, cmd, next_cmd, 		\
+					  cmd_wrapped_cnt))		\
  		pr_err_ratelimited("ITS cmd %ps failed\n", builder);	\
  }

-- 
1.8.3