On Mon, Feb 12, 2024 at 5:13 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> On 2/12/24 06:12, Aleksandr Mezin wrote:
> > Instead of pre-allocating a buffer and synchronizing the access to it,
> > simply allocate memory when needed.
> >
> > Fewer synchronization primitives, fewer lines of code.
> >
>
> Trading that for runtime overhead and an additional failure point ?
Because it's a USB device, hid_hw_output_report() calls
usbhid_output_report() -> usb_interrupt_msg() -> usb_bulk_msg() ->
usb_alloc_urb() -> kmalloc(). So there's already the same failure
point, and the overhead is already there, no?
Honestly, I didn't think too much about performance - because I expect
such devices to send and receive not more than 10 reports per second.
I don't insist on this change, I just want to understand when to
prefer simplicity vs potential performance.
> I don't think that is super-valuable, and it doesn't really save
> anything in the execution path except making it more expensive.
>
> You could save as many lines of code by allocating the buffer
> as part of struct kraken3_data, i.e., with
> u8 buffer[MAX_REPORT_LENGTH];
> instead of
> u8 *buffer;
>
> I really dislike temporary memory allocations like that, due to the
> added runtime overhead, added failure risk, and the potential for
> memory fragmentation, so unless you provide a much better reason
> for this change I am not going to accept it.
>
> Guenter
>
> > Signed-off-by: Aleksandr Mezin <mezin.alexander@xxxxxxxxx>
> > ---
> > drivers/hwmon/nzxt-kraken3.c | 19 ++++++-------------
> > 1 file changed, 6 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/hwmon/nzxt-kraken3.c b/drivers/hwmon/nzxt-kraken3.c
> > index 5806a3f32bcb..1b2aacf9f44c 100644
> > --- a/drivers/hwmon/nzxt-kraken3.c
> > +++ b/drivers/hwmon/nzxt-kraken3.c
> > @@ -97,7 +97,6 @@ struct kraken3_data {
> > struct hid_device *hdev;
> > struct device *hwmon_dev;
> > struct dentry *debugfs;
> > - struct mutex buffer_lock; /* For locking access to buffer */
> > struct mutex z53_status_request_lock;
> > struct completion fw_version_processed;
> > /*
> > @@ -109,7 +108,6 @@ struct kraken3_data {
> > /* For locking the above completion */
> > spinlock_t status_completion_lock;
> >
> > - u8 *buffer;
> > struct kraken3_channel_info channel_info[2]; /* Pump and fan */
> > bool is_device_faulty;
> >
> > @@ -186,13 +184,15 @@ static umode_t kraken3_is_visible(const void *data, enum hwmon_sensor_types type
> > static int kraken3_write_expanded(struct kraken3_data *priv, const u8 *cmd, int cmd_length)
> > {
> > int ret;
> > + u8 *buffer = kmalloc(MAX_REPORT_LENGTH, GFP_KERNEL);
> >
> > - mutex_lock(&priv->buffer_lock);
> > + if (buffer == NULL)
> > + return -ENOMEM;
> >
> > - memcpy_and_pad(priv->buffer, MAX_REPORT_LENGTH, cmd, cmd_length, 0x00);
> > - ret = hid_hw_output_report(priv->hdev, priv->buffer, MAX_REPORT_LENGTH);
> > + memcpy_and_pad(buffer, MAX_REPORT_LENGTH, cmd, cmd_length, 0x00);
> > + ret = hid_hw_output_report(priv->hdev, buffer, MAX_REPORT_LENGTH);
> >
> > - mutex_unlock(&priv->buffer_lock);
> > + kfree(buffer);
> > return ret;
> > }
> >
> > @@ -913,13 +913,6 @@ static int kraken3_probe(struct hid_device *hdev, const struct hid_device_id *id
> > break;
> > }
> >
> > - priv->buffer = devm_kzalloc(&hdev->dev, MAX_REPORT_LENGTH, GFP_KERNEL);
> > - if (!priv->buffer) {
> > - ret = -ENOMEM;
> > - goto fail_and_close;
> > - }
> > -
> > - mutex_init(&priv->buffer_lock);
> > mutex_init(&priv->z53_status_request_lock);
> > init_completion(&priv->fw_version_processed);
> > init_completion(&priv->status_report_processed);
>
