| Message ID | 20240212141311.110808-1-mezin.alexander@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | hwmon: (nzxt-kraken3) Remove buffer_lock | expand |
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 ? 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@gmail.com> > --- > 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);
On Mon, Feb 12, 2024 at 5:13 PM Guenter Roeck <linux@roeck-us.net> 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@gmail.com> > > --- > > 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); >
On 2/12/24 07:50, Aleksandr Mezin wrote: > On Mon, Feb 12, 2024 at 5:13 PM Guenter Roeck <linux@roeck-us.net> 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. > We'll have to agree to disagree that we have a different understanding of "simplicity". Guenter
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);
Instead of pre-allocating a buffer and synchronizing the access to it, simply allocate memory when needed. Fewer synchronization primitives, fewer lines of code. Signed-off-by: Aleksandr Mezin <mezin.alexander@gmail.com> --- drivers/hwmon/nzxt-kraken3.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-)