* Re: [PATCH] irqchip/gic-v3-its: handle rd_idx wrapping in its_wait_for_range_completion()
2018-02-11 18:45 ` [PATCH] irqchip/gic-v3-its: handle rd_idx wrapping in its_wait_for_range_completion() Marc Zyngier
@ 2018-03-01 1:53 ` Yang Yingliang
2018-03-06 7:06 ` [RFC PATCH] irqchip/gic-v3-its: handle wrapped case " Yang Yingliang
1 sibling, 0 replies; 3+ messages in thread
From: Yang Yingliang @ 2018-03-01 1:53 UTC (permalink / raw)
To: Marc Zyngier; +Cc: linux-arm-kernel, LKML
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.
>
^ permalink raw reply [flat|nested] 3+ messages in thread* [RFC PATCH] irqchip/gic-v3-its: handle wrapped case in its_wait_for_range_completion()
2018-02-11 18:45 ` [PATCH] irqchip/gic-v3-its: handle rd_idx wrapping in its_wait_for_range_completion() Marc Zyngier
2018-03-01 1:53 ` Yang Yingliang
@ 2018-03-06 7:06 ` Yang Yingliang
1 sibling, 0 replies; 3+ messages in thread
From: Yang Yingliang @ 2018-03-06 7:06 UTC (permalink / raw)
To: Marc Zyngier; +Cc: linux-arm-kernel, LKML, Yangyingliang
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
^ permalink raw reply related [flat|nested] 3+ messages in thread