RE: [PATCH v4 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently
Hi, Rafael
> From: Rafael J. Wysocki [mailto:r...@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
> > Signed-off-by: Lv Zheng
> > ---
> > 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 "0x";
Then "if (validation_count >= ACPI_MAX_TABLE_VALIDATIONS)" matches as
"validation_count == 0x(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 == 0x(ACPI_MAX_TABLE_VALIDATIONS)" now
the rest of this function will be skipped just like return_VOID.
B. validation_count == 0x:
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 0x (in this case,
overflowed/underflowed values are same).
A.2. "if (validation_count == ACPI_MAX_TABLE_VALIDATIONS)" doesn't match as
"validation_count == 0x" now
the rest of this funct
Re: [PATCH v4 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently
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
> > Signed-off-by: Lv Zheng
> > ---
> > 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
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
> Signed-off-by: Lv Zheng
> ---
> 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
[PATCH v4 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently
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
Signed-off-by: Lv Zheng
---
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)
{
+ 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 */
--
2.7.4
