Re: [PATCH v8 20/22] counter: Implement events_queue_size sysfs attribute
From: William Breathitt Gray
Date: Fri Feb 26 2021 - 19:21:41 EST
On Fri, Feb 26, 2021 at 06:14:12PM -0600, David Lechner wrote:
>
On 2/25/21 6:03 PM, William Breathitt Gray wrote:
>
> On Sun, Feb 21, 2021 at 03:51:40PM +0000, Jonathan Cameron wrote:
>
>> On 2021年2月18日 19:32:16 +0900
>
>> William Breathitt Gray <vilhelm.gray@xxxxxxxxx> wrote:
>
>>
>
>>> On Sun, Feb 14, 2021 at 06:11:46PM +0000, Jonathan Cameron wrote:
>
>>>> On 2021年2月12日 21:13:44 +0900
>
>>>> William Breathitt Gray <vilhelm.gray@xxxxxxxxx> wrote:
>
>>>>
>
>>>>> The events_queue_size sysfs attribute provides a way for users to
>
>>>>> dynamically configure the Counter events queue size for the Counter
>
>>>>> character device interface. The size is in number of struct
>
>>>>> counter_event data structures. The number of elements will be rounded-up
>
>>>>> to a power of 2 due to a requirement of the kfifo_alloc function called
>
>>>>> during reallocation of the queue.
>
>>>>>
>
>>>>> Cc: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
>
>>>>> Signed-off-by: William Breathitt Gray <vilhelm.gray@xxxxxxxxx>
>
>>>>> ---
>
>>>>> Documentation/ABI/testing/sysfs-bus-counter | 8 +++++++
>
>>>>> drivers/counter/counter-chrdev.c | 23 +++++++++++++++++++
>
>>>>> drivers/counter/counter-chrdev.h | 2 ++
>
>>>>> drivers/counter/counter-sysfs.c | 25 +++++++++++++++++++++
>
>>>>> 4 files changed, 58 insertions(+)
>
>>>>>
>
>>>>> diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
>
>>>>> index 847e96f19d19..f6cb2a8b08a7 100644
>
>>>>> --- a/Documentation/ABI/testing/sysfs-bus-counter
>
>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-counter
>
>>>>> @@ -212,6 +212,14 @@ Description:
>
>>>>> both edges:
>
>>>>> Any state transition.
>
>>>>>
>
>>>>> +What: /sys/bus/counter/devices/counterX/events_queue_size
>
>>>>> +KernelVersion: 5.13
>
>>>>> +Contact: linux-iio@xxxxxxxxxxxxxxx
>
>>>>> +Description:
>
>>>>> + Size of the Counter events queue in number of struct
>
>>>>> + counter_event data structures. The number of elements will be
>
>>>>> + rounded-up to a power of 2.
>
>>>>> +
>
>>>>> What: /sys/bus/counter/devices/counterX/name
>
>>>>> KernelVersion: 5.2
>
>>>>> Contact: linux-iio@xxxxxxxxxxxxxxx
>
>>>>> diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
>
>>>>> index 16f02df7f73d..53eea894e13f 100644
>
>>>>> --- a/drivers/counter/counter-chrdev.c
>
>>>>> +++ b/drivers/counter/counter-chrdev.c
>
>>>>> @@ -375,6 +375,29 @@ void counter_chrdev_remove(struct counter_device *const counter)
>
>>>>> cdev_del(&counter->chrdev);
>
>>>>> }
>
>>>>>
>
>>>>> +int counter_chrdev_realloc_queue(struct counter_device *const counter,
>
>>>>> + size_t queue_size)
>
>>>>> +{
>
>>>>> + int err;
>
>>>>> + DECLARE_KFIFO_PTR(events, struct counter_event);
>
>>>>> + unsigned long flags;
>
>>>>> +
>
>>>>> + /* Allocate new events queue */
>
>>>>> + err = kfifo_alloc(&events, queue_size, GFP_ATOMIC);
>
>>>>
>
>>>> Is there any potential for losing events?
>
>>>
>
>>> We take the events_list_lock down below so we're safe against missing an
>
>>> event, but past events currently unread in the queue will be lost.
>
>>>
>
>>> Shortening the size of the queue is inherently a destructive process if
>
>>> we have more events in the current queue than can fit in the new queue.
>
>>> Because we a liable to lose some events in such a case, I think it's
>
>>> best to keep the behavior of this reallocation consistent and have it
>
>>> provide a fresh empty queue every time, as opposed to sometimes dropping
>
>>> events and sometimes not.
>
>>>
>
>>> I also suspect an actual user would be setting the size of their queue
>
>>> to the required amount before they begin watching events, rather than
>
>>> adjusting it sporadically during a live operation.
>
>>>
>
>>
>
>> Absolutely agree. As such I wonder if you are better off enforcing this
>
>> behaviour? If the cdev is open for reading, don't allow the fifo to be
>
>> resized.
>
>>
>
>> Jonathan
>
>
>
> I can't really think of a good reason not to, so let's enforce it: if
>
> the cdev is open, then we'll return an EINVAL if the user attempts to
>
> resize the queue.
>
>
>
> What is a good way to check for this condition? Should I just call
>
> kref_read() and see if it's greater than 1? For example, in
>
> counter_chrdev_realloc_queue():
>
>
>
> if (kref_read(&counter->dev.kobj.kref) > 1)
>
> return -EINVAL;
>
>
>
> William Breathitt Gray
>
>
>
>
Wouldn't EBUSY make more sense?
Yes, EBUSY would be better here because the operation isn't necessarily
invalid, just unavailable because someone else has the cdev open.
William Breathitt Gray
Attachment:
signature.asc
Description: PGP signature