netdev - RE: [PATCH v2 4/6] ethtool: support per-queue sub command --show-coalesce

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 7 Feb 2019 23:53:40 +0000
From:   "Nunley, Nicholas D" <nicholas.d.nunley@...el.com>
To:     Michal Kubecek <mkubecek@...e.cz>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
        "linville@...driver.com" <linville@...driver.com>,
        "nhorman@...hat.com" <nhorman@...hat.com>,
        "sassmann@...hat.com" <sassmann@...hat.com>
Subject: RE: [PATCH v2 4/6] ethtool: support per-queue sub command
 --show-coalesce

> -----Original Message-----
> From: Michal Kubecek [mailto:mkubecek@...e.cz]
> Sent: Wednesday, February 6, 2019 5:22 AM
> To: netdev@...r.kernel.org
> Cc: Kirsher, Jeffrey T <jeffrey.t.kirsher@...el.com>; linville@...driver.com;
> Nunley, Nicholas D <nicholas.d.nunley@...el.com>; nhorman@...hat.com;
> sassmann@...hat.com
> Subject: Re: [PATCH v2 4/6] ethtool: support per-queue sub command --
> show-coalesce
> 
> On Tue, Feb 05, 2019 at 04:01:04PM -0800, Jeff Kirsher wrote:
> > diff --git a/ethtool.c b/ethtool.c
> > index 4dc725c..9a1b83b 100644
> > --- a/ethtool.c
> > +++ b/ethtool.c
> > @@ -1409,6 +1409,29 @@ static int dump_coalesce(const struct
> ethtool_coalesce *ecoal)
> >  	return 0;
> >  }
> >
> > +void dump_per_queue_coalesce(struct ethtool_per_queue_op
> *per_queue_opt,
> > +			     __u32 *queue_mask)
> > +{
> > +	char *addr;
> > +	int i;
> > +
> > +	addr = (char *)per_queue_opt + sizeof(*per_queue_opt);
> > +	for (i = 0; i < __KERNEL_DIV_ROUND_UP(MAX_NUM_QUEUE, 32);
> i++) {
> > +		int queue = i * 32;
> > +		__u32 mask = queue_mask[i];
> > +
> > +		while (mask > 0) {
> > +			if (mask & 0x1) {
> > +				fprintf(stdout, "Queue: %d\n", queue);
> > +				dump_coalesce((struct ethtool_coalesce
> *)addr);
> > +				addr += sizeof(struct ethtool_coalesce);
> > +			}
> > +			mask = mask >> 1;
> > +			queue++;
> > +		}
> > +	}
> > +}
> > +
> >  struct feature_state {
> >  	u32 off_flags;
> >  	struct ethtool_gfeatures features;
> 
> The casts and pointer arithmetic are a bit complicated. How about this?
> 
> 	struct ethtool_coalesce *addr;
> ...
> 	addr = (struct ethtool_coalesce *)(per_queue_opt + 1); ...
> 	dump_coalesce(addr);
> 	addr++;
> 
> Also passing n_queue down from do_perqueue() would prevent having to
> parse the whole bitmap even if we already know the NIC has only few
> queues.

Okay, that does make the pointer arithmetic a little more straightforward, so I'll change this. I'll also add the n_queue parameter to set_per_queue_coalesce() and dump_per_queue_coalesce() so we can exit early once we've found all the queues.

> > @@ -5245,7 +5268,8 @@ static const struct option {
> >  	{ "--show-fec", 1, do_gfec, "Show FEC settings"},
> >  	{ "--set-fec", 1, do_sfec, "Set FEC settings",
> >  	  "		[ encoding auto|off|rs|baser [...]]\n"},
> > -	{ "--set-perqueue-command", 1, do_perqueue, "Set per queue
> command",
> > +	{ "--set-perqueue-command", 1, do_perqueue, "Set per queue
> command. "
> > +	  "The supported sub commands include --show-coalesce",
> >  	  "             [queue_mask %x] SUB_COMMAND\n"},
> >  	{ "-h|--help", 0, show_usage, "Show this help" },
> >  	{ "--version", 0, do_version, "Show version number" }, @@ -5350,8
> > +5374,32 @@ static int find_max_num_queues(struct cmd_context *ctx)
> >  		   echannels.combined_count);
> >  }
> >
> > +static struct ethtool_per_queue_op *
> > +get_per_queue_coalesce(struct cmd_context *ctx, __u32 *queue_mask,
> > +int n_queues) {
> > +	struct ethtool_per_queue_op *per_queue_opt;
> > +
> > +	per_queue_opt = malloc(sizeof(*per_queue_opt) + n_queues *
> > +			sizeof(struct ethtool_coalesce));
> > +	if (!per_queue_opt)
> > +		return NULL;
> > +
> > +	memcpy(per_queue_opt->queue_mask, queue_mask,
> > +	       __KERNEL_DIV_ROUND_UP(MAX_NUM_QUEUE, 32) *
> sizeof(__u32));
> > +	per_queue_opt->cmd = ETHTOOL_PERQUEUE;
> > +	per_queue_opt->sub_command = ETHTOOL_GCOALESCE;
> > +	if (send_ioctl(ctx, per_queue_opt)) {
> > +		free(per_queue_opt);
> > +		perror("Cannot get device per queue parameters");
> > +		return NULL;
> > +	}
> > +
> > +	return per_queue_opt;
> > +}
> > +
> >  static int do_perqueue(struct cmd_context *ctx)  {
> > +	struct ethtool_per_queue_op *per_queue_opt;
> >  	__u32
> queue_mask[__KERNEL_DIV_ROUND_UP(MAX_NUM_QUEUE, 32)] = {0};
> >  	int i, n_queues = 0;
> >
> > @@ -5390,7 +5438,19 @@ static int do_perqueue(struct cmd_context *ctx)
> >  	if (i < 0)
> >  		exit_bad_args();
> >
> > -	/* no sub_command support yet */
> > +	if (strstr(args[i].opts, "--show-coalesce") != NULL) {
> 
> Comparing args[i].func to do_gcoalesce might be easier.

This is the one comment where I think it's better to leave the code as it is. To me is seems more confusing to match on a function pointer that we're never going to call. Unless there are more objections I'd rather keep it the way it is.

Otherwise, thanks for the review and all the comments. I'll work on making these changes and get an updated version submitted.

Nick

> 
> > +		per_queue_opt = get_per_queue_coalesce(ctx,
> queue_mask,
> > +						       n_queues);
> > +		if (per_queue_opt == NULL) {
> > +			perror("Cannot get device per queue parameters");
> > +			return -EFAULT;
> > +		}
> > +		dump_per_queue_coalesce(per_queue_opt, queue_mask);
> > +		free(per_queue_opt);
> > +	} else {
> > +		perror("The subcommand is not supported yet");
> > +		return -EOPNOTSUPP;
> > +	}
> >
> >  	return 0;
> >  }
> 
> Michal Kubecek