Re: [RFC 1/5] net/ethtool: introduce a new ioctl for per queue setting

[Ben Hutchings]

On Thu, 2015-12-17 at 06:51 +0000, kan.li...@intel.com wrote:
> From: Kan Liang <kan.li...@intel.com>
> 
> Introduce a new ioctl ETHTOOL_PERQUEUE for per queue parameters setting.
> The following patches will enable some SUB_COMMANDs for per queue
> setting.
> 
> Signed-off-by: Kan Liang <kan.li...@intel.com>
> ---
>  include/uapi/linux/ethtool.h | 18 ++++++++++++++++++
>  net/core/ethtool.c           | 17 +++++++++++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index cd16291..05bc92a 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -1144,6 +1144,22 @@ enum ethtool_sfeatures_retval_bits {
>  #define ETHTOOL_F_WISH          (1 << ETHTOOL_F_WISH__BIT)
>  #define ETHTOOL_F_COMPAT        (1 << ETHTOOL_F_COMPAT__BIT)
>  
> +#define MAX_QUEUE            4096
> +#define MAX_QUEUE_MASK               (MAX_QUEUE / 64)
> +
> +/**
> + * struct ethtool_per_queue_op - apply sub command to the queues in mask.
> + * @cmd: ETHTOOL_PERQUEUE
> + * @queue_mask: Mask the queues which sub command apply to
> + * @sub_command: the sub command
> + * @data: parameters of the command
> + */
> +struct ethtool_per_queue_op {
> +     __u32   cmd;
> +     __u64   queue_mask[MAX_QUEUE_MASK];
> +     __u32   sub_command;

This leaves a hole in the structure on i386, which would then require
conversion in the compat ioctl implementation.  I suggest you move the
sub_command next to cmd.

> +     char    data[];
> +};
>  
>  /* CMDs currently supported */
>  #define ETHTOOL_GSET         0x00000001 /* Get settings. */
> @@ -1226,6 +1242,8 @@ enum ethtool_sfeatures_retval_bits {
>  #define ETHTOOL_GTUNABLE     0x00000048 /* Get tunable configuration */
>  #define ETHTOOL_STUNABLE     0x00000049 /* Set tunable configuration */
>  
> +#define ETHTOOL_PERQUEUE     0x0000004a /* Set per queue options */

But it will actually be used for both set and get, right?

> +
>  /* compatibility with older code */
>  #define SPARC_ETH_GSET               ETHTOOL_GSET
>  #define SPARC_ETH_SSET               ETHTOOL_SSET
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 29edf74..125fb32 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -1748,6 +1748,20 @@ out:
>       return ret;
>  }
>  
> +static int ethtool_set_per_queue(struct net_device *dev, void __user 
> *useraddr)
> +{
> +     struct ethtool_per_queue_op per_queue_opt;
> +
> +     if (copy_from_user(&per_queue_opt, useraddr, sizeof(per_queue_opt)))
> +             return -EFAULT;
> +
> +     switch (per_queue_opt.sub_command) {
> +
> +     default:
> +             return -EOPNOTSUPP;
> +     };
> +}
> +
>  /* The main entry point in this file.  Called from net/core/dev_ioctl.c */
>  
>  int dev_ethtool(struct net *net, struct ifreq *ifr)
> @@ -1991,6 +2005,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
>       case ETHTOOL_STUNABLE:
>               rc = ethtool_set_tunable(dev, useraddr);
>               break;
> +     case ETHTOOL_PERQUEUE:
> +             rc = ethtool_set_per_queue(dev, useraddr);
> +             break;
>       default:
>               rc = -EOPNOTSUPP;
>       }

I think the first switch statement to determine whether the command
requires CAP_NET_ADMIN should also handle ETHTOOL_PERQUEUE, checking
the sub-command number.

Ben.

-- 
Ben Hutchings
Always try to do things in chronological order;
it's less confusing that way.

signature.asc
Description: This is a digitally signed message part