Messages in this thread |  | | Subject | Re: [PATCH] counter: drop chrdev_lock | From | David Lechner <> | Date | Tue, 19 Oct 2021 09:44:04 -0500 |
| |
On 10/19/21 2:18 AM, William Breathitt Gray wrote: > On Tue, Oct 19, 2021 at 09:07:48AM +0200, Greg KH wrote: >> On Tue, Oct 19, 2021 at 03:53:08PM +0900, William Breathitt Gray wrote: >>> On Mon, Oct 18, 2021 at 11:03:49AM -0500, David Lechner wrote: >>>> On 10/18/21 4:14 AM, William Breathitt Gray wrote: >>>>> On Sun, Oct 17, 2021 at 01:55:21PM -0500, David Lechner wrote: >>>>>> diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c >>>>>> index 1ccd771da25f..7bf8882ff54d 100644 >>>>>> --- a/drivers/counter/counter-sysfs.c >>>>>> +++ b/drivers/counter/counter-sysfs.c >>>>>> @@ -796,25 +796,18 @@ static int counter_events_queue_size_write(struct counter_device *counter, >>>>>> u64 val) >>>>>> { >>>>>> DECLARE_KFIFO_PTR(events, struct counter_event); >>>>>> - int err = 0; >>>>>> - >>>>>> - /* Ensure chrdev is not opened more than 1 at a time */ >>>>>> - if (!atomic_add_unless(&counter->chrdev_lock, 1, 1)) >>>>>> - return -EBUSY; >>>>>> + int err; >>>>>> >>>>>> /* Allocate new events queue */ >>>>>> err = kfifo_alloc(&events, val, GFP_KERNEL); >>>>>> if (err) >>>>>> - goto exit_early; >>>>>> + return err; >>>>>> >>>>>> /* Swap in new events queue */ >>>>>> kfifo_free(&counter->events); >>>>>> counter->events.kfifo = events.kfifo; >>>>> >>>>> Do we need to hold the events_lock mutex here for this swap in case >>>>> counter_chrdev_read() is in the middle of reading the kfifo to >>>>> userspace, or do the kfifo macros already protect us from a race >>>>> condition here? >>>>> >>>> Another possibility might be to disallow changing the size while >>>> events are enabled. Otherwise, we also need to protect against >>>> write after free. >>>> >>>> I considered this: >>>> >>>> swap(counter->events.kfifo, events.kfifo); >>>> kfifo_free(&events); >>>> >>>> But I'm not sure that would be safe enough. >>> >>> I think it depends on whether it's safe to call kfifo_free() while other >>> kfifo_*() calls are executing. I suspect it is not safe because I don't >>> think kfifo_free() waits until all kfifo read/write operations are >>> finished before freeing -- but if I'm wrong here please let me know. >>> >>> Because of that, will need to hold the counter->events_lock afterall so >>> that we don't modify the events fifo while a kfifo read/write is going >>> on, lest we suffer an address fault. This can happen regardless of >>> whether you swap before or after the kfifo_free() because the old fifo >>> address could still be in use within those uncompleted kfifo_*() calls >>> if they were called before the swap but don't complete before the >>> kfifo_free(). >>> >>> So we have a problem now that I think you have already noticed: the >>> kfifo_in() call in counter_push_events() also needs protection, but it's >>> executing within an interrupt context so we can't try to lock a mutex >>> lest we end up sleeping. >>> >>> One option we have is as you suggested: we disallow changing size while >>> events are enabled. However, that will require us to keep track of when >>> events are disabled and implement a spinlock to ensure that we don't >>> disable events in the middle of a kfifo_in(). >>> >>> Alternatively, we could change events_lock to a spinlock and use it to >>> protect all these operations on the counter->events fifo. Would this >>> alternative be a better option so that we avoid creating another >>> separate lock? >> >> I would recommend just having a single lock here if at all possible, >> until you determine that there a performance problem that can be >> measured that would require it to be split up. >> >> thanks, >> >> greg k-h > > All right let's go with a single events_lock spinlock then. David, if > you make those changes and submit a v2, I'll be okay with this patch and > can provide my ack for it. >
We can't use a spin lock for everything since there are operations that can sleep that need to be in the critical sections. Likewise, we can't use a mutex for everything since some critical sections are in interrupt handlers. But, I suppose we can try combining the existing mutexes. Since the kfifo is accessed from both contexts, it seems like it still needs more consideration than just a mutex or a spin lock, e.g. if events are enabled, don't allow swapping out the kfifo buffer.
|  |