Virtualization - Jan 2014 - [PATCH net-next v2 4/4] virtio-net: initial debugfs support, export mergeable rx buffer size

Hi Michael,

On Sun, Jan 12, 2014 at 9:09 AM, Michael S. Tsirkin <mst at redhat.com>
wrote:
> Can't we add struct attribute * to netdevice, and pass that in when
> creating the kobj?
I like that idea, I think that will work and should be better than
the alternatives. The actual kobjs for RX queues (struct netdev_rx_queue)
are allocated and deallocated by calls to net_rx_queue_update_kobjects,
which resizes RX queue kobjects when the netdev RX queues are resized.

Is this what you had in mind:
(1) Add a pointer to an attribute group to struct net_device, used for
    per-netdev rx queue attributes and initialized before the call to
    register_netdevice().
(2) Declare an attribute group containing the mergeable_rx_buffer_size
    attribute in virtio-net, and initialize the per-netdevice group pointer
    to the address of this group in virtnet_probe before register_netdevice
(3) In net-sysfs, modify net_rx_queue_update_kobjects
    (or rx_queue_add_kobject) to call sysfs_create_group on the
    per-netdev attribute group (if non-NULL), adding the attributes in
    the group to the RX queue kobject.

That should allow us to have per-RX queue attributes that are
device-specific. I'm not a sysfs expert, but it seems that rx_queue_ktype
and rx_queue_sysfs_ops presume that all rx queue sysfs operations are
performed on attributes of type rx_queue_attribute. That type will need
to be moved from net-sysfs.c to a header file like netdevice.h so that
the type can be used in virtio-net when we declare the
mergeable_rx_buffer_size attribute.

The last issue is how the rx_queue_attribute 'show' function
implementation for mergeable_rx_buffer_size will access the appropriate
per-receive queue EWMA data. The arguments to the show function will be
the netdev_rx_queue and the attribute itself. We can get to the
struct net_device from the netdev_rx_queue.  If we extended
netdev_rx_queue to indicate the queue_index or to store a void *priv_data
pointer, that would be sufficient to allow us to resolve this issue.

Please let me know if the above sounds good or if you see a better way
to accomplish this goal. Thanks!

Best,

Mike

On 01/13/2014 07:32 AM, Michael Dalton wrote:
> Hi Michael,
>
> On Sun, Jan 12, 2014 at 9:09 AM, Michael S. Tsirkin <mst at
redhat.com> wrote:
>> Can't we add struct attribute * to netdevice, and pass that in when
>> creating the kobj?
> I like that idea, I think that will work and should be better than
> the alternatives. The actual kobjs for RX queues (struct netdev_rx_queue)
> are allocated and deallocated by calls to net_rx_queue_update_kobjects,
> which resizes RX queue kobjects when the netdev RX queues are resized.
>
> Is this what you had in mind:
> (1) Add a pointer to an attribute group to struct net_device, used for
>     per-netdev rx queue attributes and initialized before the call to
>     register_netdevice().
> (2) Declare an attribute group containing the mergeable_rx_buffer_size
>     attribute in virtio-net, and initialize the per-netdevice group pointer
>     to the address of this group in virtnet_probe before register_netdevice
> (3) In net-sysfs, modify net_rx_queue_update_kobjects
>     (or rx_queue_add_kobject) to call sysfs_create_group on the
>     per-netdev attribute group (if non-NULL), adding the attributes in
>     the group to the RX queue kobject.
>
> That should allow us to have per-RX queue attributes that are
> device-specific. I'm not a sysfs expert, but it seems that
rx_queue_ktype
> and rx_queue_sysfs_ops presume that all rx queue sysfs operations are
> performed on attributes of type rx_queue_attribute. That type will need
> to be moved from net-sysfs.c to a header file like netdevice.h so that
> the type can be used in virtio-net when we declare the
> mergeable_rx_buffer_size attribute.
There's a possible issue, rx queue sysfs depends on CONFIG_RPS. So we
probably need a dedicated attribute just for virtio-net.
>
> The last issue is how the rx_queue_attribute 'show' function
> implementation for mergeable_rx_buffer_size will access the appropriate
> per-receive queue EWMA data. The arguments to the show function will be
> the netdev_rx_queue and the attribute itself. We can get to the
> struct net_device from the netdev_rx_queue.  If we extended
> netdev_rx_queue to indicate the queue_index or to store a void *priv_data
> pointer, that would be sufficient to allow us to resolve this issue.
>
> Please let me know if the above sounds good or if you see a better way
> to accomplish this goal. Thanks!
>
> Best,
>
> Mike

On Sun, Jan 12, 2014 at 03:32:28PM -0800, Michael Dalton
wrote:
> Hi Michael,
> 
> On Sun, Jan 12, 2014 at 9:09 AM, Michael S. Tsirkin <mst at
redhat.com> wrote:
> > Can't we add struct attribute * to netdevice, and pass that in
when
> > creating the kobj?
> 
> I like that idea, I think that will work and should be better than
> the alternatives. The actual kobjs for RX queues (struct netdev_rx_queue)
> are allocated and deallocated by calls to net_rx_queue_update_kobjects,
> which resizes RX queue kobjects when the netdev RX queues are resized.
> 
> Is this what you had in mind:
> (1) Add a pointer to an attribute group to struct net_device, used for
>     per-netdev rx queue attributes and initialized before the call to
>     register_netdevice().
> (2) Declare an attribute group containing the mergeable_rx_buffer_size
>     attribute in virtio-net, and initialize the per-netdevice group pointer
>     to the address of this group in virtnet_probe before register_netdevice
> (3) In net-sysfs, modify net_rx_queue_update_kobjects
>     (or rx_queue_add_kobject) to call sysfs_create_group on the
>     per-netdev attribute group (if non-NULL), adding the attributes in
>     the group to the RX queue kobject.

Exactly.
> That should allow us to have per-RX queue attributes that are
> device-specific. I'm not a sysfs expert, but it seems that
rx_queue_ktype
> and rx_queue_sysfs_ops presume that all rx queue sysfs operations are
> performed on attributes of type rx_queue_attribute. That type will need
> to be moved from net-sysfs.c to a header file like netdevice.h so that
> the type can be used in virtio-net when we declare the
> mergeable_rx_buffer_size attribute.
> 
> The last issue is how the rx_queue_attribute 'show' function
> implementation for mergeable_rx_buffer_size will access the appropriate
> per-receive queue EWMA data. The arguments to the show function will be
> the netdev_rx_queue and the attribute itself. We can get to the
> struct net_device from the netdev_rx_queue.  If we extended
> netdev_rx_queue to indicate the queue_index or to store a void *priv_data
> pointer, that would be sufficient to allow us to resolve this issue.
Hmm netdev_rx_queue is not defined unless CONFIG_RPS is set.
Maybe we should use a different structure.

> Please let me know if the above sounds good or if you see a better way
> to accomplish this goal. Thanks!
> 
> Best,
> 
> Mike
Sounds good to me.

On Mon, 2014-01-13 at 11:40 +0200, Michael S. Tsirkin
wrote:
> On Sun, Jan 12, 2014 at 03:32:28PM -0800, Michael Dalton wrote:
[...]
> > The last issue is how the rx_queue_attribute 'show' function
> > implementation for mergeable_rx_buffer_size will access the
appropriate
> > per-receive queue EWMA data. The arguments to the show function will
be
> > the netdev_rx_queue and the attribute itself. We can get to the
> > struct net_device from the netdev_rx_queue.  If we extended
> > netdev_rx_queue to indicate the queue_index or to store a void
*priv_data
> > pointer, that would be sufficient to allow us to resolve this issue.
> 
> Hmm netdev_rx_queue is not defined unless CONFIG_RPS is set.
> Maybe we should use a different structure.
[...]

I don't think RPS should own this structure.  It's just that there are
currently no per-RX-queue attributes other than those defined by RPS.

By the way, CONFIG_RPS is equivalent to CONFIG_SMP so will likely be
enabled already in most places where virtio_net is used.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.