From: Ben Hutchings <b...@decadent.org.uk>
Date: Tue, 08 Dec 2015 19:00:52 +0000
> On Tue, 2015-12-08 at 10:02 -0800, Shannon Nelson wrote:
>> On Mon, Dec 7, 2015 at 8:42 PM, <kan.li...@intel.com> wrote:
>> > From: Kan Liang <kan.li...@intel.com>
>> >
>> > Intrdouce "queue" option for coalesce getting and setting.
> [...]
>> > --- a/net/core/ethtool.c
>> > +++ b/net/core/ethtool.c
>> > @@ -1123,10 +1123,16 @@ static noinline_for_stack int
>> > ethtool_get_coalesce(struct net_device *dev,
>> > void __user *useraddr)
>> > {
>> > struct ethtool_coalesce coalesce = { .cmd = ETHTOOL_GCOALESCE };
>> > + struct ethtool_coalesce tmp = { .queue = -1 };
>> >
>> > if (!dev->ethtool_ops->get_coalesce)
>> > return -EOPNOTSUPP;
>> >
>> > + if (copy_from_user(&tmp, useraddr, sizeof(coalesce)))
>> > + return -EFAULT;
>> > +
>> > + coalesce.queue = tmp.queue;
>> > +
>>
>> Is this going to do what you expect when you have an older ethtool
>> program? It seems to me that the older ethtool program will have the
>> original coalesce struct without the new queue field, so when you do
> [...]
>
> Indeed, it's not safe to extend UAPI structures like this.
Agreed, and this is really very far from what I suggested.
Please, make a new ethtool command entirely, that is a container.
Name is 'ETHTOOL_PERQUEUE' or something like that.
It's base type is a structure that describes what queue the
encapsulated operation is to be applied to.
struct ethtool_per_queue_op {
u32 queue_mask[BITS_TO_LONGS(WHATEVER)];
u32 sub_command;
char data[];
};
Then sub_command can be ETHTOOL_SCOALESCE, or any other ethtool
command that might be valid to be specified on a per-queue basis.
And then at 'data' you have and array of objects, whose type is of
the sub-command's expected type, and has one entry for each bit
set in queue_mask.
If you limit this facility to only ETHTOOL_{S,G}COALESCE now, believe
me you'll regret it.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
