| Message ID | 86628bf775497ed03931e30361bb6115d78876be.1494309338.git.lv.zheng@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Tuesday, May 09, 2017 01:57:41 PM Lv Zheng wrote: > For all frequent late stage acpi_get_table() clone invocations, we should > only change them altogether, otherwise, excessive acpi_put_table() could > unexpectedly unmap the table used by the other users. Thus the current plan > is to change all acpi_get_table() clones together or to change none of > them. However in practical, this is not convenient as this can prevent > kernel developers' efforts of improving the late stage code quality before > waiting for the ACPICA upstream to improve first. > > This patch adds a validation count threashold, when it is reached, the > validation count can no longer be incremented/decremented to invalidate the > table descriptor (means preventing table unmappings) so that acpi_put_table() > balance changes can be done independently to each others. Lv Zheng. > > Cc: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > --- > drivers/acpi/acpica/tbutils.c | 24 +++++++++++++++--------- > include/acpi/actbl.h | 9 +++++++++ > 2 files changed, 24 insertions(+), 9 deletions(-) > > diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c > index 7abe665..04beafc 100644 > --- a/drivers/acpi/acpica/tbutils.c > +++ b/drivers/acpi/acpica/tbutils.c > @@ -416,9 +416,13 @@ acpi_tb_get_table(struct acpi_table_desc *table_desc, > } > } > > - table_desc->validation_count++; > - if (table_desc->validation_count == 0) { > - table_desc->validation_count--; > + if (table_desc->validation_count < ACPI_MAX_TABLE_VALIDATIONS) { > + table_desc->validation_count++; > + if (table_desc->validation_count >= ACPI_MAX_TABLE_VALIDATIONS) { > + ACPI_WARNING((AE_INFO, > + "Table %p, Validation count overflows\n", > + table_desc)); > + } > } > > *out_table = table_desc->pointer; > @@ -445,13 +449,15 @@ void acpi_tb_put_table(struct acpi_table_desc *table_desc) > > ACPI_FUNCTION_TRACE(acpi_tb_put_table); > > - if (table_desc->validation_count == 0) { > - ACPI_WARNING((AE_INFO, > - "Table %p, Validation count is zero before decrement\n", > - table_desc)); > - return_VOID; > + if (table_desc->validation_count < ACPI_MAX_TABLE_VALIDATIONS) { > + table_desc->validation_count--; > + if (table_desc->validation_count >= ACPI_MAX_TABLE_VALIDATIONS) { Is this going to ever trigger? We've already verified that validation_count is not 0 and that it is less than ACPI_MAX_TABLE_VALIDATIONS and we have decremented it, so how can it be greater than or equal to ACPI_MAX_TABLE_VALIDATIONS here? > + ACPI_WARNING((AE_INFO, > + "Table %p, Validation count underflows\n", > + table_desc)); > + return_VOID; > + } > } > - table_desc->validation_count--; > > if (table_desc->validation_count == 0) { > Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, May 12, 2017 11:03:52 PM Rafael J. Wysocki wrote: > On Tuesday, May 09, 2017 01:57:41 PM Lv Zheng wrote: > > For all frequent late stage acpi_get_table() clone invocations, we should > > only change them altogether, otherwise, excessive acpi_put_table() could > > unexpectedly unmap the table used by the other users. Thus the current plan > > is to change all acpi_get_table() clones together or to change none of > > them. However in practical, this is not convenient as this can prevent > > kernel developers' efforts of improving the late stage code quality before > > waiting for the ACPICA upstream to improve first. > > > > This patch adds a validation count threashold, when it is reached, the > > validation count can no longer be incremented/decremented to invalidate the > > table descriptor (means preventing table unmappings) so that acpi_put_table() > > balance changes can be done independently to each others. Lv Zheng. > > > > Cc: Dan Williams <dan.j.williams@intel.com> > > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > > --- > > drivers/acpi/acpica/tbutils.c | 24 +++++++++++++++--------- > > include/acpi/actbl.h | 9 +++++++++ > > 2 files changed, 24 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c > > index 7abe665..04beafc 100644 > > --- a/drivers/acpi/acpica/tbutils.c > > +++ b/drivers/acpi/acpica/tbutils.c > > @@ -416,9 +416,13 @@ acpi_tb_get_table(struct acpi_table_desc *table_desc, > > } > > } > > > > - table_desc->validation_count++; > > - if (table_desc->validation_count == 0) { > > - table_desc->validation_count--; > > + if (table_desc->validation_count < ACPI_MAX_TABLE_VALIDATIONS) { > > + table_desc->validation_count++; > > + if (table_desc->validation_count >= ACPI_MAX_TABLE_VALIDATIONS) { > > + ACPI_WARNING((AE_INFO, > > + "Table %p, Validation count overflows\n", > > + table_desc)); > > + } > > } > > > > *out_table = table_desc->pointer; > > @@ -445,13 +449,15 @@ void acpi_tb_put_table(struct acpi_table_desc *table_desc) > > > > ACPI_FUNCTION_TRACE(acpi_tb_put_table); > > > > - if (table_desc->validation_count == 0) { > > - ACPI_WARNING((AE_INFO, > > - "Table %p, Validation count is zero before decrement\n", > > - table_desc)); > > - return_VOID; > > + if (table_desc->validation_count < ACPI_MAX_TABLE_VALIDATIONS) { > > + table_desc->validation_count--; > > + if (table_desc->validation_count >= ACPI_MAX_TABLE_VALIDATIONS) { > > Is this going to ever trigger? > > We've already verified that validation_count is not 0 and that it is less than > ACPI_MAX_TABLE_VALIDATIONS and we have decremented it, so how can it be > greater than or equal to ACPI_MAX_TABLE_VALIDATIONS here? Wrong question, sorry. I think that the check is in case validation_count was 0 before the decrementation, right? So then, I'd still check if validation_count == 0 and if so, set it to ACPI_MAX_TABLE_VALIDATIONS. Next, if validation_count => ACPI_MAX_TABLE_VALIDATIONS, I'd print the warning message and return. Then, the decrementation would not underflow, so it would be safe to do it. Wouldn't that be somewhat easier to follow? > > + ACPI_WARNING((AE_INFO, > > + "Table %p, Validation count underflows\n", > > + table_desc)); > > + return_VOID; > > + } > > } > > - table_desc->validation_count--; > > > > if (table_desc->validation_count == 0) { > > Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Rafael > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net] > Subject: Re: [PATCH v4 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage > acpi_get_table() independently > > On Tuesday, May 09, 2017 01:57:41 PM Lv Zheng wrote: > > For all frequent late stage acpi_get_table() clone invocations, we should > > only change them altogether, otherwise, excessive acpi_put_table() could > > unexpectedly unmap the table used by the other users. Thus the current plan > > is to change all acpi_get_table() clones together or to change none of > > them. However in practical, this is not convenient as this can prevent > > kernel developers' efforts of improving the late stage code quality before > > waiting for the ACPICA upstream to improve first. > > > > This patch adds a validation count threashold, when it is reached, the > > validation count can no longer be incremented/decremented to invalidate the > > table descriptor (means preventing table unmappings) so that acpi_put_table() > > balance changes can be done independently to each others. Lv Zheng. > > > > Cc: Dan Williams <dan.j.williams@intel.com> > > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > > --- > > drivers/acpi/acpica/tbutils.c | 24 +++++++++++++++--------- > > include/acpi/actbl.h | 9 +++++++++ > > 2 files changed, 24 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c > > index 7abe665..04beafc 100644 > > --- a/drivers/acpi/acpica/tbutils.c > > +++ b/drivers/acpi/acpica/tbutils.c > > @@ -416,9 +416,13 @@ acpi_tb_get_table(struct acpi_table_desc *table_desc, > > } > > } > > > > - table_desc->validation_count++; > > - if (table_desc->validation_count == 0) { > > - table_desc->validation_count--; > > + if (table_desc->validation_count < ACPI_MAX_TABLE_VALIDATIONS) { > > + table_desc->validation_count++; > > + if (table_desc->validation_count >= ACPI_MAX_TABLE_VALIDATIONS) { > > + ACPI_WARNING((AE_INFO, > > + "Table %p, Validation count overflows\n", > > + table_desc)); > > + } > > } > > > > *out_table = table_desc->pointer; > > @@ -445,13 +449,15 @@ void acpi_tb_put_table(struct acpi_table_desc *table_desc) > > > > ACPI_FUNCTION_TRACE(acpi_tb_put_table); > > > > - if (table_desc->validation_count == 0) { > > - ACPI_WARNING((AE_INFO, > > - "Table %p, Validation count is zero before decrement\n", > > - table_desc)); > > - return_VOID; > > + if (table_desc->validation_count < ACPI_MAX_TABLE_VALIDATIONS) { > > + table_desc->validation_count--; > > + if (table_desc->validation_count >= ACPI_MAX_TABLE_VALIDATIONS) { > > Is this going to ever trigger? > > We've already verified that validation_count is not 0 and that it is less than > ACPI_MAX_TABLE_VALIDATIONS and we have decremented it, so how can it be > greater than or equal to ACPI_MAX_TABLE_VALIDATIONS here? This is just a no-op change equivalent to " if (validation_count == 0) { warn and return } decrement " It expands "decrement" to "validation_count == 0" case so that it can implement warn_once for the warning message. See: A. validation_count == 0: A.1. "if (validation_count < ACPI_MAX_TABLE_VALIDATIONS)" matches, and After decrementing validation_count, it will be "0xFFFF"; Then "if (validation_count >= ACPI_MAX_TABLE_VALIDATIONS)" matches as "validation_count == 0xFFFF(ACPI_MAX_TABLE_VALIDATIONS)" now; The warning message is printed; A.2. "if (validation_count == ACPI_MAX_TABLE_VALIDATIONS)" doesn't match as "validation_count == 0xFFFF(ACPI_MAX_TABLE_VALIDATIONS)" now the rest of this function will be skipped just like return_VOID. B. validation_count == 0xFFFF: A.1. Both acpi_tb_get_table() and acpi_tb_put_table() won't be able to change validation_count as validation_count increment/decrement code fragments are only executed "if (validation_count < ACPI_MAX_TABLE_VALIDATIONS)" Thus validation_count is kept as 0xFFFF (in this case, overflowed/underflowed values are same). A.2. "if (validation_count == ACPI_MAX_TABLE_VALIDATIONS)" doesn't match as "validation_count == 0xFFFF" now the rest of this function will be skipped just like return_VOID. C. otherwise, validation_count will be decremented like old code Benefits of using the new algorithm are: 1. ACPI_MAX_TABLE_VALIDATIONS can be something other than 0xFFFF. It can be anything now, and the expected behavior can always be ensured. IOW, the new algorithm actually supports cases where overflowed/underflowed values are not same. You can check this by defining ACPI_MAX_TABLE_VALIDATIONS to 8. And you'll see that the 2 functions are still working. 1.1. If something is broken in acpi_tb_get_table(), validation_count will be kept as 0x0008. 1.2. If something is broken in acpi_tb_put_table(), validation_count will be kept as 0xFFFF. Both 0x0008 and 0xFFFF cannot make "if (validation_count < ACPI_MAX_TABLE_VALIDATIONS)" and "if (validation_count == 0)" to return true, and Thus validation_count is kept unchanged after overflow/underflow. 2. The key benefit of this change is to make the old warning in acpi_tb_put_table() as warn_once. For example: acpi_get_table(); for (i = 0; i < 100; i++) acpi_put_table() Using the old algorithm, the acpi_tb_put_table() warning message will be seen 99 times. Using the new algorithm, the acpi_tb_put_table() warning message will be seen only once. 3. logics in acpi_tb_put_table() will be exactly the reversal of the logics in acpi_tb_get_table(). It'll be easier to maintain both of them with the new overflow/underflow algorithm. Hope you'll like such a change. Thanks and best regards Lv > > > + ACPI_WARNING((AE_INFO, > > + "Table %p, Validation count underflows\n", > > + table_desc)); > > + return_VOID; > > + } > > } > > - table_desc->validation_count--; > > > > if (table_desc->validation_count == 0) { > > > > Thanks, > Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c index 7abe665..04beafc 100644 --- a/drivers/acpi/acpica/tbutils.c +++ b/drivers/acpi/acpica/tbutils.c @@ -416,9 +416,13 @@ acpi_tb_get_table(struct acpi_table_desc *table_desc, } } - table_desc->validation_count++; - if (table_desc->validation_count == 0) { - table_desc->validation_count--; + if (table_desc->validation_count < ACPI_MAX_TABLE_VALIDATIONS) { + table_desc->validation_count++; + if (table_desc->validation_count >= ACPI_MAX_TABLE_VALIDATIONS) { + ACPI_WARNING((AE_INFO, + "Table %p, Validation count overflows\n", + table_desc)); + } } *out_table = table_desc->pointer; @@ -445,13 +449,15 @@ void acpi_tb_put_table(struct acpi_table_desc *table_desc) ACPI_FUNCTION_TRACE(acpi_tb_put_table); - if (table_desc->validation_count == 0) { - ACPI_WARNING((AE_INFO, - "Table %p, Validation count is zero before decrement\n", - table_desc)); - return_VOID; + if (table_desc->validation_count < ACPI_MAX_TABLE_VALIDATIONS) { + table_desc->validation_count--; + if (table_desc->validation_count >= ACPI_MAX_TABLE_VALIDATIONS) { + ACPI_WARNING((AE_INFO, + "Table %p, Validation count underflows\n", + table_desc)); + return_VOID; + } } - table_desc->validation_count--; if (table_desc->validation_count == 0) { diff --git a/include/acpi/actbl.h b/include/acpi/actbl.h index d92543f..8e1bff8 100644 --- a/include/acpi/actbl.h +++ b/include/acpi/actbl.h @@ -374,6 +374,15 @@ struct acpi_table_desc { u16 validation_count; }; +/* + * Maximum validation count, when it is reached, validation count can no + * longer be changed. Which means, the table can no longer be invalidated. + * This mechanism is implemented for backward compatibility, where in OS + * late stage, old drivers are not facilitated with paired validations and + * invalidations. + */ +#define ACPI_MAX_TABLE_VALIDATIONS 5 + /* Masks for Flags field above */ #define ACPI_TABLE_ORIGIN_EXTERNAL_VIRTUAL (0) /* Virtual address, external maintained */
For all frequent late stage acpi_get_table() clone invocations, we should only change them altogether, otherwise, excessive acpi_put_table() could unexpectedly unmap the table used by the other users. Thus the current plan is to change all acpi_get_table() clones together or to change none of them. However in practical, this is not convenient as this can prevent kernel developers' efforts of improving the late stage code quality before waiting for the ACPICA upstream to improve first. This patch adds a validation count threashold, when it is reached, the validation count can no longer be incremented/decremented to invalidate the table descriptor (means preventing table unmappings) so that acpi_put_table() balance changes can be done independently to each others. Lv Zheng. Cc: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- drivers/acpi/acpica/tbutils.c | 24 +++++++++++++++--------- include/acpi/actbl.h | 9 +++++++++ 2 files changed, 24 insertions(+), 9 deletions(-)