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

On 01/07/2014 01:25 PM, Michael Dalton wrote:
> Add initial support for debugfs to virtio-net. Each virtio-net network
> device will have a directory under /virtio-net in debugfs. The
> per-network device directory will contain one sub-directory per active,
> enabled receive queue. If mergeable receive buffers are enabled, each
> receive queue directory will contain a read-only file that returns the
> current packet buffer size for the receive queue.
>
> Signed-off-by: Michael Dalton <mwdalton at google.com>
This looks more complicated than expected. How about just adding an
entry in sysfs onto the existed network class device which looks more
simpler?

On Wed, Jan 08, 2014 at 02:34:31PM +0800, Jason Wang
wrote:
> On 01/07/2014 01:25 PM, Michael Dalton wrote:
> > Add initial support for debugfs to virtio-net. Each virtio-net network
> > device will have a directory under /virtio-net in debugfs. The
> > per-network device directory will contain one sub-directory per
active,
> > enabled receive queue. If mergeable receive buffers are enabled, each
> > receive queue directory will contain a read-only file that returns the
> > current packet buffer size for the receive queue.
> >
> > Signed-off-by: Michael Dalton <mwdalton at google.com>
> 
> This looks more complicated than expected. How about just adding an
> entry in sysfs onto the existed network class device which looks more
> simpler?
sysfs is part of userspace ABI, I think that's the main issue: can we
commit to this attribute being there in the future?
If yes we can use sysfs but maybe it seems reasonable to use debugfs for
a while until we are more sure of this.
I don't mind either way.

Hi Jason, Michael

Sorry for the delay in response. Jason, I agree this patch ended up
being larger than expected. The major implementation parts are:
(1) Setup directory structure (driver/per-netdev/rx-queue directories)
(2) Network device renames (optional, so debugfs dir has the right name)
(3) Support resizing the # of RX queues (optional - we could just export
    max_queue_pairs files and not delete files if an RX queue is disabled)
(4) Reference counting - used in case someone opens a debugfs
    file and then removes the virtio-net device.
(5) The actual mergeable rx buffer file implementation itself. For now
    I have added a seqcount for memory safety, but if a read-only race
    condition is acceptable we could elide the seqcount. FWIW, the
    seqcount write in receive_mergeable() should, on modern x86,
    translate to two non-atomic adds and two compiler barriers, so
    overhead is not expected to be meaningful.

We can move to sysfs and this would simplify or eliminate much of the
above, including most of (1) - (4). I believe our choices for what to
do for the next patchset include:
(a) Use debugfs as is currently done, removing any optional features
listed above that are deemed unnecessary.

(b) Add a per-netdev sysfs attribute group to net_device->sysfs_groups.
Each attribute would display the mergeable packet buffer size for a given
RX queue, and there would be max_queue_pairs attributes in total. This
is already supported by net/core/net-sysfs.c:netdev_register_kobject(),
but means that we would have a static set of per-RX queue files for
all RX queues supported by the netdev, rather than dynamically displaying
only the files corresponding to enabled RX queues (e.g., when # of RX
queues is changed by  ethtool -L <device>).  For an example of this
approach, see drivers/net/bonding/bond_sysfs.c.

(c) Modify struct netdev_rx_queue to add virtio-net EWMA fields directly,
and modify net-sysfs.c to manage the new fields. Unlike (b), this approach
supports the RX queue resizing in (3) but means putting virtio-net info
in netdev_rx_queue, which currently has only device-independent fields.

My preference would be (b): try using sysfs and adding a device-specific
attribute group to the virtio-net netdevice (stored in the existing
'sysfs_groups' field and supported by net-sysfs).  This would avoid
adding virtio-net specific information to net-sysfs. What would you
prefer (or is there a better way than the approaches above)? Thanks!

Best,

Mike