linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Leon Romanovsky <leon@kernel.org>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	Saeed Mahameed <saeedm@nvidia.com>,
	Jason Gunthorpe <jgg@nvidia.com>,
	Alexander Duyck <alexander.duyck@gmail.com>,
	Jakub Kicinski <kuba@kernel.org>,
	linux-pci@vger.kernel.org, linux-rdma@vger.kernel.org,
	netdev@vger.kernel.org, Don Dutile <ddutile@redhat.com>,
	Alex Williamson <alex.williamson@redhat.com>,
	"David S . Miller" <davem@davemloft.net>
Subject: Re: [PATCH mlx5-next v6 1/4] PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs
Date: Tue, 16 Feb 2021 09:33:44 +0200	[thread overview]
Message-ID: <YCt1WAAEO1hx2pjY@unreal> (raw)
In-Reply-To: <20210215210106.GA744958@bjorn-Precision-5520>

On Mon, Feb 15, 2021 at 03:01:06PM -0600, Bjorn Helgaas wrote:
> On Tue, Feb 09, 2021 at 03:34:42PM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> >
> > Extend PCI sysfs interface with a new callback that allows configuration
> > of the number of MSI-X vectors for specific SR-IOV VF. This is needed
> > to optimize the performance of VFs devices by allocating the number of
> > vectors based on the administrator knowledge of the intended use of the VF.
> >
> > This function is applicable for SR-IOV VF because such devices allocate
> > their MSI-X table before they will run on the VMs and HW can't guess the
> > right number of vectors, so some devices allocate them statically and equally.
>
> This commit log should be clear that this functionality is motivated
> by *mlx5* behavior.  The description above makes it sound like this is
> generic PCI spec behavior, and it is not.
>
> It may be a reasonable design that conforms to the spec, and we hope
> the model will be usable by other designs, but it is not required by
> the spec and AFAIK there is nothing in the spec you can point to as
> background for this.
>
> So don't *remove* the text you have above, but please *add* some
> preceding background information about how mlx5 works.
>
> > 1) The newly added /sys/bus/pci/devices/.../sriov_vf_msix_count
> > file will be seen for the VFs and it is writable as long as a driver is not
> > bound to the VF.
>
>   This adds /sys/bus/pci/devices/.../sriov_vf_msix_count for VF
>   devices and is writable ...
>
> > The values accepted are:
> >  * > 0 - this will be number reported by the Table Size in the VF's MSI-X Message
> >          Control register
> >  * < 0 - not valid
> >  * = 0 - will reset to the device default value
>
>   = 0 - will reset to a device-specific default value
>
> > 2) In order to make management easy, provide new read-only sysfs file that
> > returns a total number of possible to configure MSI-X vectors.
>
>   For PF devices, this adds a read-only
>   /sys/bus/pci/devices/.../sriov_vf_total_msix file that contains the
>   total number of MSI-X vectors available for distribution among VFs.
>
> Just as in sysfs-bus-pci, this file should be listed first, because
> you must read it before you can use vf_msix_count.

No problem, I'll change, just remember that we are talking about commit
message because in Documentation file, the order is exactly as you request.

>
> > cat /sys/bus/pci/devices/.../sriov_vf_total_msix
> >   = 0 - feature is not supported
> >   > 0 - total number of MSI-X vectors available for distribution among the VFs
> >
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-pci |  28 +++++
> >  drivers/pci/iov.c                       | 153 ++++++++++++++++++++++++
> >  include/linux/pci.h                     |  12 ++
> >  3 files changed, 193 insertions(+)

<...>

> > + */
> > +int pci_enable_vf_overlay(struct pci_dev *dev)
> > +{
> > +	struct pci_dev *virtfn;
> > +	int id, ret;
> > +
> > +	if (!dev->is_physfn || !dev->sriov->num_VFs)
> > +		return 0;
> > +
> > +	ret = sysfs_create_files(&dev->dev.kobj, sriov_pf_dev_attrs);
>
> But I still don't like the fact that we're calling
> sysfs_create_files() and sysfs_remove_files() directly.  It makes
> complication and opportunities for errors.

It is not different from any other code that we have in the kernel.
Let's be concrete, can you point to the errors in this code that I
should fix?

>
> I don't see the advantage of creating these files only when the PF
> driver supports this.  The management tools have to deal with
> sriov_vf_total_msix == 0 and sriov_vf_msix_count == 0 anyway.
> Having the sysfs files not be present at all might be slightly
> prettier to the person running "ls", but I'm not sure the code
> complication is worth that.

It is more than "ls", right now sriov_numvfs is visible without relation
to the driver, even if driver doesn't implement ".sriov_configure", which
IMHO bad. We didn't want to repeat.

Right now, we have many devices that supports SR-IOV, but small amount
of them are capable to rewrite their VF MSI-X table siz. We don't want
"to punish" and clatter their sysfs.

>
> I see a hint that Alex might have requested this "only visible when PF
> driver supports it" functionality, but I don't see that email on
> linux-pci, so I missed the background.

First version of this patch had static files solution.
https://lore.kernel.org/linux-pci/20210103082440.34994-2-leon@kernel.org/#Z30drivers:pci:iov.c

Thanks

  reply	other threads:[~2021-02-16  7:34 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-09 13:34 [PATCH mlx5-next v6 0/4] Dynamically assign MSI-X vectors count Leon Romanovsky
2021-02-09 13:34 ` [PATCH mlx5-next v6 1/4] PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs Leon Romanovsky
2021-02-15 21:01   ` Bjorn Helgaas
2021-02-16  7:33     ` Leon Romanovsky [this message]
2021-02-16 16:12       ` Bjorn Helgaas
2021-02-16 19:58         ` Leon Romanovsky
2021-02-17 18:02           ` Bjorn Helgaas
2021-02-17 19:25             ` Jason Gunthorpe
2021-02-17 20:28               ` Bjorn Helgaas
2021-02-17 23:52                 ` Jason Gunthorpe
2021-02-18 10:15             ` Leon Romanovsky
2021-02-18 22:39               ` Bjorn Helgaas
2021-02-19  7:52                 ` Leon Romanovsky
2021-02-19  8:20                   ` Greg Kroah-Hartman
2021-02-19 16:58                     ` Leon Romanovsky
2021-02-20 19:06                     ` Bjorn Helgaas
2021-02-21  6:59                       ` Leon Romanovsky
2021-02-23 21:07                         ` Bjorn Helgaas
2021-02-24  8:09                           ` Greg Kroah-Hartman
2021-02-24 21:37                             ` Don Dutile
2021-02-24  9:53                           ` Leon Romanovsky
2021-02-24 15:07                             ` Bjorn Helgaas
2021-02-21 13:00                       ` Greg Kroah-Hartman
2021-02-21 13:55                         ` Leon Romanovsky
2021-02-21 15:01                           ` Greg Kroah-Hartman
2021-02-21 15:30                             ` Leon Romanovsky
2021-02-16 20:37         ` Jason Gunthorpe
2021-02-09 13:34 ` [PATCH mlx5-next v6 2/4] net/mlx5: Add dynamic MSI-X capabilities bits Leon Romanovsky
2021-02-09 13:34 ` [PATCH mlx5-next v6 3/4] net/mlx5: Dynamically assign MSI-X vectors count Leon Romanovsky
2021-02-09 13:34 ` [PATCH mlx5-next v6 4/4] net/mlx5: Allow to the users to configure number of MSI-X vectors Leon Romanovsky
2021-02-09 21:06 ` [PATCH mlx5-next v6 0/4] Dynamically assign MSI-X vectors count Alexander Duyck
2021-02-14  5:24   ` Leon Romanovsky

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YCt1WAAEO1hx2pjY@unreal \
    --to=leon@kernel.org \
    --cc=alex.williamson@redhat.com \
    --cc=alexander.duyck@gmail.com \
    --cc=bhelgaas@google.com \
    --cc=davem@davemloft.net \
    --cc=ddutile@redhat.com \
    --cc=helgaas@kernel.org \
    --cc=jgg@nvidia.com \
    --cc=kuba@kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-rdma@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=saeedm@nvidia.com \
    --subject='Re: [PATCH mlx5-next v6 1/4] PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).