[lkml]   [2021]   [Oct]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH] counter: drop chrdev_lock
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.

 \ /
  Last update: 2021-10-19 16:44    [W:0.100 / U:0.900 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site