Re: [Devel] ref-counting races?

[Andriy Gapon <avg at FreeBSD.org>]

[-- Attachment #1: Type: text/plain, Size: 1946 bytes --]

on 22/03/2013 22:23 Moore, Robert said the following:
> Andriy,
> 
> This will of course take a bit of time for us to completely sort out. You are
> correct that there are unlocked calls to the reference count mechanism. This
> could be a result of some code restructuring sometime in the past, as I hope
> that this didn't happen as an oversight.
> 
> I want to investigate some older code as well as the original design of the
> reference counts.

Thank you!

> If we do need a separate lock for the reference count mechanism, perhaps a
> spin lock could be used instead of a mutex, as there is nothing that will
> block for any length of time. This may not make any difference on FreeBSD as
> spinlocks may not be available, I'm not sure.

I agree that a spinlock would be more appropriate.  Just in case, FreeBSD does
provide ACPICA spinlock implementation.

> In the meantime, I have removed the REF_FORCE_DELETE option as per your
> suggestion; it was implemented in case it was ever needed, which it has not.

Thank you again.

Do you perhaps have an advice on an interim solution for FreeBSD users who seem
to keep bumping into this issue?
I will have a look into trying to prevent concurrency at the battery driver
level.  To, at least, reduce the chances of hitting the problem.

In fact, I'll ask the user who helped me, kron24(a)gmail.com, to test the
following (rather coarse) patch:
--- a/sys/dev/acpica/acpi_battery.c
+++ b/sys/dev/acpica/acpi_battery.c
@@ -360,6 +360,8 @@ acpi_battery_ioctl(u_long cmd, caddr_t addr, void *arg)
     int error, unit;
     device_t dev;

+    mtx_lock(&Giant);
+
     /* For commands that use the ioctl_arg struct, validate it first. */
     error = ENXIO;
     unit = 0;
@@ -417,6 +419,7 @@ acpi_battery_ioctl(u_long cmd, caddr_t addr, void *arg)
 	error = EINVAL;
     }

+    mtx_unlock(&Giant);
     return (error);
 }


-- 
Andriy Gapon