Re: [Bugfix v2 2/5] x86/irq: Enhance __assign_irq_vector() to rollback in case of failure

[Thomas Gleixner]

On Wed, 23 Dec 2015, Jiang Liu wrote:
> @@ -167,11 +170,13 @@ next:
>  
>   if (test_bit(vector, used_vectors))
>   goto next;
> -
>   for_each_cpu_and(new_cpu, vector_cpumask, cpu_online_mask) {
>   if (!IS_ERR_OR_NULL(per_cpu(vector_irq, 
> new_cpu)[vector]))
>   goto next;
>   }
> + if (apic->cpu_mask_to_apicid_and(mask, vector_cpumask, ))
> + goto next;

This is silly. If the checks in cpu_mask_to_apicid_and() fail, then there is
no point to try the next vector number simply because the checks will fail
with the next vector again. You need a new vector_cpumask() in order to make
progress.

> +
>   /* Found one! */
>   current_vector = vector;
>   current_offset = offset;
> @@ -190,8 +195,7 @@ next:
>  
>   if (!err) {
>   /* cache destination APIC IDs into cfg->dest_apicid */
> - err = apic->cpu_mask_to_apicid_and(mask, d->domain,
> ->cfg.dest_apicid);

I have serious doubts that this is the proper solution.

d->domain is @vector_cpumask. @vector_cpumask gets assigned by the call to
apic->vector_allocation_domain()

So the only way this can fail is when:

   vector_cpumask & mask & cpu_online_mask == 0

So we can do that check way earlier, i.e. right after the call to
apic->vector_allocation_domain(). Once we have established that this check
does not fail, we know that the call to apic->cpu_mask_to_apicid_and() wont
fail either once we have a vector number to use.

Thanks,

tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Bugfix v2 2/5] x86/irq: Enhance __assign_irq_vector() to rollback in case of failure

[Thomas Gleixner]

On Wed, 23 Dec 2015, Jiang Liu wrote:
> @@ -167,11 +170,13 @@ next:
>  
>   if (test_bit(vector, used_vectors))
>   goto next;
> -
>   for_each_cpu_and(new_cpu, vector_cpumask, cpu_online_mask) {
>   if (!IS_ERR_OR_NULL(per_cpu(vector_irq, 
> new_cpu)[vector]))
>   goto next;
>   }
> + if (apic->cpu_mask_to_apicid_and(mask, vector_cpumask, ))
> + goto next;

This is silly. If the checks in cpu_mask_to_apicid_and() fail, then there is
no point to try the next vector number simply because the checks will fail
with the next vector again. You need a new vector_cpumask() in order to make
progress.

> +
>   /* Found one! */
>   current_vector = vector;
>   current_offset = offset;
> @@ -190,8 +195,7 @@ next:
>  
>   if (!err) {
>   /* cache destination APIC IDs into cfg->dest_apicid */
> - err = apic->cpu_mask_to_apicid_and(mask, d->domain,
> ->cfg.dest_apicid);

I have serious doubts that this is the proper solution.

d->domain is @vector_cpumask. @vector_cpumask gets assigned by the call to
apic->vector_allocation_domain()

So the only way this can fail is when:

   vector_cpumask & mask & cpu_online_mask == 0

So we can do that check way earlier, i.e. right after the call to
apic->vector_allocation_domain(). Once we have established that this check
does not fail, we know that the call to apic->cpu_mask_to_apicid_and() wont
fail either once we have a vector number to use.

Thanks,

tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Bugfix v2 2/5] x86/irq: Enhance __assign_irq_vector() to rollback in case of failure

[Jiang Liu]

Enhance __assign_irq_vector() to rollback in case of failure so the
caller doesn't need to explicitly rollback.

Signed-off-by: Jiang Liu 
---
 arch/x86/kernel/apic/vector.c |   26 --
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
index d6ec36b4461e..b32c6ef7b4b0 100644
--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -117,6 +117,7 @@ static int __assign_irq_vector(int irq, struct 
apic_chip_data *d,
static int current_vector = FIRST_EXTERNAL_VECTOR + VECTOR_OFFSET_START;
static int current_offset = VECTOR_OFFSET_START % 16;
int cpu, err;
+   unsigned int dest;
 
if (d->move_in_progress)
return -EBUSY;
@@ -132,19 +133,21 @@ static int __assign_irq_vector(int irq, struct 
apic_chip_data *d,
apic->vector_allocation_domain(cpu, vector_cpumask, mask);
 
if (cpumask_subset(vector_cpumask, d->domain)) {
-   err = 0;
-   if (cpumask_equal(vector_cpumask, d->domain))
-   break;
/*
 * New cpumask using the vector is a proper subset of
 * the current in use mask. So cleanup the vector
 * allocation for the members that are not used anymore.
 */
+   cpumask_and(used_cpumask, d->domain, vector_cpumask);
+   err = apic->cpu_mask_to_apicid_and(mask, used_cpumask,
+  );
+   if (err || cpumask_equal(vector_cpumask, d->domain))
+   break;
cpumask_andnot(d->old_domain, d->domain,
   vector_cpumask);
d->move_in_progress =
   cpumask_intersects(d->old_domain, cpu_online_mask);
-   cpumask_and(d->domain, d->domain, vector_cpumask);
+   cpumask_copy(d->domain, used_cpumask);
break;
}
 
@@ -167,11 +170,13 @@ next:
 
if (test_bit(vector, used_vectors))
goto next;
-
for_each_cpu_and(new_cpu, vector_cpumask, cpu_online_mask) {
if (!IS_ERR_OR_NULL(per_cpu(vector_irq, 
new_cpu)[vector]))
goto next;
}
+   if (apic->cpu_mask_to_apicid_and(mask, vector_cpumask, ))
+   goto next;
+
/* Found one! */
current_vector = vector;
current_offset = offset;
@@ -190,8 +195,7 @@ next:
 
if (!err) {
/* cache destination APIC IDs into cfg->dest_apicid */
-   err = apic->cpu_mask_to_apicid_and(mask, d->domain,
-  >cfg.dest_apicid);
+   d->cfg.dest_apicid = dest;
}
 
return err;
@@ -493,14 +497,8 @@ static int apic_set_affinity(struct irq_data *irq_data,
return -EINVAL;
 
err = assign_irq_vector(irq, data, dest);
-   if (err) {
-   if (assign_irq_vector(irq, data,
- irq_data_get_affinity_mask(irq_data)))
-   pr_err("Failed to recover vector for irq %d\n", irq);
-   return err;
-   }
 
-   return IRQ_SET_MASK_OK;
+   return err ? err : IRQ_SET_MASK_OK;
 }
 
 static struct irq_chip lapic_controller = {
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Bugfix v2 2/5] x86/irq: Enhance __assign_irq_vector() to rollback in case of failure

[Jiang Liu]

Enhance __assign_irq_vector() to rollback in case of failure so the
caller doesn't need to explicitly rollback.

Signed-off-by: Jiang Liu 
---
 arch/x86/kernel/apic/vector.c |   26 --
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
index d6ec36b4461e..b32c6ef7b4b0 100644
--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -117,6 +117,7 @@ static int __assign_irq_vector(int irq, struct 
apic_chip_data *d,
static int current_vector = FIRST_EXTERNAL_VECTOR + VECTOR_OFFSET_START;
static int current_offset = VECTOR_OFFSET_START % 16;
int cpu, err;
+   unsigned int dest;
 
if (d->move_in_progress)
return -EBUSY;
@@ -132,19 +133,21 @@ static int __assign_irq_vector(int irq, struct 
apic_chip_data *d,
apic->vector_allocation_domain(cpu, vector_cpumask, mask);
 
if (cpumask_subset(vector_cpumask, d->domain)) {
-   err = 0;
-   if (cpumask_equal(vector_cpumask, d->domain))
-   break;
/*
 * New cpumask using the vector is a proper subset of
 * the current in use mask. So cleanup the vector
 * allocation for the members that are not used anymore.
 */
+   cpumask_and(used_cpumask, d->domain, vector_cpumask);
+   err = apic->cpu_mask_to_apicid_and(mask, used_cpumask,
+  );
+   if (err || cpumask_equal(vector_cpumask, d->domain))
+   break;
cpumask_andnot(d->old_domain, d->domain,
   vector_cpumask);
d->move_in_progress =
   cpumask_intersects(d->old_domain, cpu_online_mask);
-   cpumask_and(d->domain, d->domain, vector_cpumask);
+   cpumask_copy(d->domain, used_cpumask);
break;
}
 
@@ -167,11 +170,13 @@ next:
 
if (test_bit(vector, used_vectors))
goto next;
-
for_each_cpu_and(new_cpu, vector_cpumask, cpu_online_mask) {
if (!IS_ERR_OR_NULL(per_cpu(vector_irq, 
new_cpu)[vector]))
goto next;
}
+   if (apic->cpu_mask_to_apicid_and(mask, vector_cpumask, ))
+   goto next;
+
/* Found one! */
current_vector = vector;
current_offset = offset;
@@ -190,8 +195,7 @@ next:
 
if (!err) {
/* cache destination APIC IDs into cfg->dest_apicid */
-   err = apic->cpu_mask_to_apicid_and(mask, d->domain,
-  >cfg.dest_apicid);
+   d->cfg.dest_apicid = dest;
}
 
return err;
@@ -493,14 +497,8 @@ static int apic_set_affinity(struct irq_data *irq_data,
return -EINVAL;
 
err = assign_irq_vector(irq, data, dest);
-   if (err) {
-   if (assign_irq_vector(irq, data,
- irq_data_get_affinity_mask(irq_data)))
-   pr_err("Failed to recover vector for irq %d\n", irq);
-   return err;
-   }
 
-   return IRQ_SET_MASK_OK;
+   return err ? err : IRQ_SET_MASK_OK;
 }
 
 static struct irq_chip lapic_controller = {
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/