Re: [PATCH V4 5/8] net/ethtool: support set coalesce per queue
From: Ben Hutchings <hidden>
Date: 2016-02-14 18:55:23
On Mon, 2016-02-08 at 02:45 -0500, Kan Liang wrote:
quoted hunk ↗ jump to hunk
From: Kan Liang <redacted> This patch implements sub command ETHTOOL_SCOALESCE for ioctl ETHTOOL_PERQUEUE. It introduces an interface set_per_queue_coalesce to set coalesce of each masked queue to device driver. The wanted coalesce information are stored in "data" for each masked queue, which can copy from userspace. If it fails to set coalesce to device driver, the value which already set to specific queue will be tried to rollback. Signed-off-by: Kan Liang <redacted> --- include/linux/ethtool.h | 8 +++++++ net/core/ethtool.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+)diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index a83566f..0c699b8 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h@@ -208,6 +208,12 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)* zero should return. But related unavailable fields should be set to ~0, * which indicates RX or TX is not in the range. * Returns a negative error code or zero. + * @set_per_queue_coalesce: Set interrupt coalescing parameters per queue. + * It needs to do range check for the input queue number. Only if + * neither RX nor TX queue number is in the range, a negative error code + * returns. For the case that only RX or only TX is not in the range, + * zero should return. The related unavailable fields should be avoid. + * Returns a negative error code or zero.
This is poorly worded. I suggest: "It must check that the given queue number is valid. If neither a RX nor a TX queue has this number, return -EINVAL. If only a RX queue or a TX queue has this number, ignore the inapplicable fields."
quoted hunk ↗ jump to hunk
* * All operations are optional (i.e. the function pointer may be set * to %NULL) and callers must take this into account. Callers must@@ -288,6 +294,8 @@ struct ethtool_ops {const struct ethtool_tunable *, const void *); int (*get_per_queue_coalesce)(struct net_device *, int, struct ethtool_coalesce *); + int (*set_per_queue_coalesce)(struct net_device *, int, + struct ethtool_coalesce *);
Again, please use unsigned int for the queue number. [...]
quoted hunk ↗ jump to hunk
diff --git a/net/core/ethtool.c b/net/core/ethtool.c index ccefbf5..933de09 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c@@ -1854,6 +1854,64 @@ static int ethtool_get_per_queue_coalesce(struct net_device *dev,return 0; } +static int ethtool_set_per_queue_coalesce(struct net_device *dev, + void __user *useraddr, + struct ethtool_per_queue_op *per_queue_opt) +{ + int bit, i, ret = 0; + int queue_num;
It would be clearer to call this n_queues ('number of queues') rather
than queue_num.
+ struct ethtool_coalesce *backup = NULL, *tmp = NULL;
+ DECLARE_BITMAP(queue_mask, MAX_NUM_QUEUE);
+
+ if ((!dev->ethtool_ops->set_per_queue_coalesce) ||
+ (!dev->ethtool_ops->get_per_queue_coalesce))
+ return -EOPNOTSUPP;
+
+ useraddr += sizeof(*per_queue_opt);
+
+ bitmap_from_u32array(queue_mask,
+ MAX_NUM_QUEUE,
+ per_queue_opt->queue_mask,
+ DIV_ROUND_UP(MAX_NUM_QUEUE, 32));
+ queue_num = bitmap_weight(queue_mask, MAX_NUM_QUEUE);
+ tmp = backup = kmalloc_array(queue_num, sizeof(*backup), GFP_KERNEL);
+ if (!backup)
+ return -ENOMEM;
+
+ for_each_set_bit(bit, queue_mask, MAX_NUM_QUEUE) {
+ struct ethtool_coalesce coalesce;
+
+ ret = dev->ethtool_ops->get_per_queue_coalesce(dev, bit, tmp);
+ if (ret != 0)
+ goto roll_back;
+
+ tmp += sizeof(struct ethtool_coalesce);No! tmp is a pointer to struct ethtool_coalesce, so it must be incremented by 1.
+ if (copy_from_user(&coalesce, useraddr, sizeof(coalesce))) {
+ ret = -EFAULT;
+ goto roll_back;
+ }
+
+ ret = dev->ethtool_ops->set_per_queue_coalesce(dev, bit, &coalesce);
+ if (ret != 0)
+ goto roll_back;
+
+ useraddr += sizeof(coalesce);
+ }
+
+roll_back:
+ if (ret != 0) {
+ tmp = backup;
+ for_each_set_bit(i, queue_mask, bit) {
+ dev->ethtool_ops->set_per_queue_coalesce(dev, i, tmp);
+ tmp += sizeof(struct ethtool_coalesce);[...] Again, increment by 1. Ben. -- Ben Hutchings Sturgeon's Law: Ninety percent of everything is crap.
Attachments
- signature.asc [application/pgp-signature] 811 bytes