On Oct 21 15:40, Łukasz Gieryk wrote: > On Wed, Oct 20, 2021 at 09:06:06PM +0200, Klaus Jensen wrote: > > On Oct 7 18:24, Lukasz Maniak wrote: > > > From: Łukasz Gieryk <lukasz.gie...@linux.intel.com> > > > > > > The Nvme device defines two properties: max_ioqpairs, msix_qsize. Having > > > them as constants is problematic for SR-IOV support. > > > > > > The SR-IOV feature introduces virtual resources (queues, interrupts) > > > that can be assigned to PF and its dependent VFs. Each device, following > > > a reset, should work with the configured number of queues. A single > > > constant is no longer sufficient to hold the whole state. > > > > > > This patch tries to solve the problem by introducing additional > > > variables in NvmeCtrl’s state. The variables for, e.g., managing queues > > > are therefore organized as: > > > > > > - n->params.max_ioqpairs – no changes, constant set by the user. > > > > > > - n->max_ioqpairs - (new) value derived from n->params.* in realize(); > > > constant through device’s lifetime. > > > > > > - n->(mutable_state) – (not a part of this patch) user-configurable, > > > specifies number of queues available _after_ > > > reset. > > > > > > - n->conf_ioqpairs - (new) used in all the places instead of the ‘old’ > > > n->params.max_ioqpairs; initialized in realize() > > > and updated during reset() to reflect user’s > > > changes to the mutable state. > > > > > > Since the number of available i/o queues and interrupts can change in > > > runtime, buffers for sq/cqs and the MSIX-related structures are > > > allocated big enough to handle the limits, to completely avoid the > > > complicated reallocation. A helper function (nvme_update_msixcap_ts) > > > updates the corresponding capability register, to signal configuration > > > changes. > > > > > > Signed-off-by: Łukasz Gieryk <lukasz.gie...@linux.intel.com> > > > > Instead of this, how about adding new parameters, say, sriov_vi_private > > and sriov_vq_private. Then, max_ioqpairs and msix_qsize are still the > > "physical" limits and the new parameters just reserve some for the > > primary controller, the rest being available for flexsible resources. > > Compare your configuration: > > max_ioqpairs = 26 > sriov_max_vfs = 4 > sriov_vq_private = 10 > > with mine: > > max_ioqpairs = 10 > sriov_max_vfs = 4 > sriov_max_vq_per_vf = 4 > > In your version, if I wanted to change max_vfs but keep the same number > of flexible resources per VF, then I would have to do some math and > update max_ioparis. And then I also would have to adjust the other > interrupt-related parameter, as it's also affected. In my opinion > it's quite inconvenient.
True, that is probably inconvenient, but we have tools to do this math for us. I very much prefer to be explicit in these parameters. Also, see my comment on patch 12. If we keep this meaning of max_ioqpairs, then we have reasonable defaults for the number of private resources (if no flexible resources are required) and I think we can control all parameters in the capabilities structures (with a little math). > > Now, even if I changed the semantic of params, I would still need most > of this patch. (Let’s keep the discussion regarding if max_* fields are > necessary in the other thread). > > Without virtualization, the maximum number of queues is constant. User > (i.e., nvme kernel driver) can only query this value (e.g., 10) and > needs to follow this limit. > > With virtualization, the flexible resources kick in. Let's continue with > the sample numbers defined earlier (10 private + 16 flexible resources). > > 1) The device boots, all 16 flexible queues are assigned to the primary > controller. > 2) Nvme kernel driver queries for the limit (10+16=26) and can create/use > up to this many queues. > 3) User via the virtualization management command unbinds some (let's > say 2) of the flexible queues from the primary controller and assigns > them to a secondary controller. > 4) After reset, the Physical Function Device reports different limit > (24), and when the Virtual Device shows up, it will report 1 (adminQ > consumed the other resource). > > So I need additional variable in the state to store the intermediate > limit (24 or 1), as none of the existing params has the correct value, > and all the places that validate limits must work on the value. > I do not contest that you need additional state to keep track of assigned resources. That seems totally reasonable.
signature.asc
Description: PGP signature