| Message ID | 20231128194215.3318925-1-jacob.e.keller@intel.com |
|---|---|
| State | Accepted |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | [iwl-next,v4] ice: replace ice_vf_recreate_vsi() with ice_vf_reconfig_vsi() | expand |
On 11/28/23 20:42, Jacob Keller wrote: > The ice_vf_create_vsi() function and its VF ops helper introduced by commit > a4c785e8162e ("ice: convert vf_ops .vsi_rebuild to .create_vsi") are used > during an individual VF reset to re-create the VSI. This was done in order > to ensure that the VSI gets properly reconfigured within the hardware. > > This is somewhat heavy handed as we completely release the VSI memory and > structure, and then create a new VSI. This can also potentially force a > change of the VSI index as we will re-use the first open slot in the VSI > array which may not be the same. > > As part of implementing devlink reload, commit 6624e780a577 ("ice: split > ice_vsi_setup into smaller functions") split VSI setup into smaller > functions, introducing both ice_vsi_cfg() and ice_vsi_decfg() which can be > used to configure or deconfigure an existing software VSI structure. > > Rather than completely removing the VSI and adding a new one using the > .create_vsi() VF operation, simply use ice_vsi_decfg() to remove the > current configuration. Save the VSI type and then call ice_vsi_cfg() to > reconfigure the VSI as the same type that it was before. > > The existing reset logic assumes that all hardware filters will be removed, > so also call ice_fltr_remove_all() before re-configuring the VSI. [here] > > This new operation does not re-create the VSI, so rename it to > ice_vf_reconfig_vsi(). > > The new approach can safely share the exact same flow for both SR-IOV VFs > as well as the Scalable IOV VFs being worked on. This uses less code and is > a better abstraction over fully deleting the VSI and adding a new one. > > Fixes: a4c785e8162e ("ice: convert vf_ops .vsi_rebuild to .create_vsi") I see no such SHA in my tree. Anyway, perhaps this patch could be targeted to -net? > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > --- > Changes since v3: > * Removal of filters was a side effect of VSI being fully deleted by > firmware. Since we don't do that now, it was not happening. Fix this by > adding a call to ice_fltr_remove_all(). Thanks to Jakub Buchocki for I could imagine giving credits also in the commit msg [here] :) > figuring out this solution. > * Rebase on to current dev-queue. > > v3 was posted at https://lore.kernel.org/intel-wired-lan/20230712222936.1048751-1-jacob.e.keller@intel.com/ > v2 was posted at https://lore.kernel.org/intel-wired-lan/20230712204730.326331-1-jacob.e.keller@intel.com/ > v1 was posted at https://lore.kernel.org/intel-wired-lan/20230504211829.3869868-1-jacob.e.keller@intel.com/ > > > drivers/net/ethernet/intel/ice/ice_sriov.c | 24 ++----------- > drivers/net/ethernet/intel/ice/ice_vf_lib.c | 35 +++++++++++++------ > drivers/net/ethernet/intel/ice/ice_vf_lib.h | 1 - > .../ethernet/intel/ice/ice_vf_lib_private.h | 1 + > 4 files changed, 28 insertions(+), 33 deletions(-) > code is fine, so: Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > base-commit: 0392e4d0c1cd583c079a18685cd308f8ce903793 that's good reason to ask for a re-test after long time, I agree! My tree also does not have this SHA, you could likely base it on current "netdev/main"
> -----Original Message----- > From: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com> > Sent: Wednesday, November 29, 2023 3:16 AM > To: Keller, Jacob E <jacob.e.keller@intel.com>; Intel Wired LAN <intel-wired- > lan@lists.osuosl.org> > Cc: Romanowski, Rafal <rafal.romanowski@intel.com>; poros > <poros@redhat.com>; Michal Swiatkowski <michal.swiatkowski@linux.intel.com>; > Buchocki, JakubX <jakubx.buchocki@intel.com>; Nguyen, Anthony L > <anthony.l.nguyen@intel.com>; Polchlopek, Mateusz > <mateusz.polchlopek@intel.com> > Subject: Re: [PATCH iwl-next v4] ice: replace ice_vf_recreate_vsi() with > ice_vf_reconfig_vsi() > > On 11/28/23 20:42, Jacob Keller wrote: > > The ice_vf_create_vsi() function and its VF ops helper introduced by commit > > a4c785e8162e ("ice: convert vf_ops .vsi_rebuild to .create_vsi") are used > > during an individual VF reset to re-create the VSI. This was done in order > > to ensure that the VSI gets properly reconfigured within the hardware. > > > > This is somewhat heavy handed as we completely release the VSI memory and > > structure, and then create a new VSI. This can also potentially force a > > change of the VSI index as we will re-use the first open slot in the VSI > > array which may not be the same. > > > > As part of implementing devlink reload, commit 6624e780a577 ("ice: split > > ice_vsi_setup into smaller functions") split VSI setup into smaller > > functions, introducing both ice_vsi_cfg() and ice_vsi_decfg() which can be > > used to configure or deconfigure an existing software VSI structure. > > > > Rather than completely removing the VSI and adding a new one using the > > .create_vsi() VF operation, simply use ice_vsi_decfg() to remove the > > current configuration. Save the VSI type and then call ice_vsi_cfg() to > > reconfigure the VSI as the same type that it was before. > > > > The existing reset logic assumes that all hardware filters will be removed, > > so also call ice_fltr_remove_all() before re-configuring the VSI. > > [here] > > > > > This new operation does not re-create the VSI, so rename it to > > ice_vf_reconfig_vsi(). > > > > The new approach can safely share the exact same flow for both SR-IOV VFs > > as well as the Scalable IOV VFs being worked on. This uses less code and is > > a better abstraction over fully deleting the VSI and adding a new one. > > > > Fixes: a4c785e8162e ("ice: convert vf_ops .vsi_rebuild to .create_vsi") > > I see no such SHA in my tree. > Anyway, perhaps this patch could be targeted to -net? > Hmm. I must have gotten the tag wrong somewhere. I don't consider this -net, because its not really a fixing a bug, it’s a behavioral cleanup which is a better solution for future. We can drop the fixes tag. Thanks, Jake > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > > --- > > Changes since v3: > > * Removal of filters was a side effect of VSI being fully deleted by > > firmware. Since we don't do that now, it was not happening. Fix this by > > adding a call to ice_fltr_remove_all(). Thanks to Jakub Buchocki for > > I could imagine giving credits also in the commit msg [here] :) > > > figuring out this solution. > > * Rebase on to current dev-queue. > > > > v3 was posted at https://lore.kernel.org/intel-wired- > lan/20230712222936.1048751-1-jacob.e.keller@intel.com/ > > v2 was posted at https://lore.kernel.org/intel-wired- > lan/20230712204730.326331-1-jacob.e.keller@intel.com/ > > v1 was posted at https://lore.kernel.org/intel-wired- > lan/20230504211829.3869868-1-jacob.e.keller@intel.com/ > > > > > > drivers/net/ethernet/intel/ice/ice_sriov.c | 24 ++----------- > > drivers/net/ethernet/intel/ice/ice_vf_lib.c | 35 +++++++++++++------ > > drivers/net/ethernet/intel/ice/ice_vf_lib.h | 1 - > > .../ethernet/intel/ice/ice_vf_lib_private.h | 1 + > > 4 files changed, 28 insertions(+), 33 deletions(-) > > > > code is fine, so: > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > > > > base-commit: 0392e4d0c1cd583c079a18685cd308f8ce903793 > > that's good reason to ask for a re-test after long time, I agree! > > My tree also does not have this SHA, you could likely base it on current > "netdev/main"
Dne 28. 11. 23 v 20:42 Jacob Keller napsal(a): > The ice_vf_create_vsi() function and its VF ops helper introduced by commit > a4c785e8162e ("ice: convert vf_ops .vsi_rebuild to .create_vsi") are used > during an individual VF reset to re-create the VSI. This was done in order > to ensure that the VSI gets properly reconfigured within the hardware. > > This is somewhat heavy handed as we completely release the VSI memory and > structure, and then create a new VSI. This can also potentially force a > change of the VSI index as we will re-use the first open slot in the VSI > array which may not be the same. > > As part of implementing devlink reload, commit 6624e780a577 ("ice: split > ice_vsi_setup into smaller functions") split VSI setup into smaller > functions, introducing both ice_vsi_cfg() and ice_vsi_decfg() which can be > used to configure or deconfigure an existing software VSI structure. > > Rather than completely removing the VSI and adding a new one using the > .create_vsi() VF operation, simply use ice_vsi_decfg() to remove the > current configuration. Save the VSI type and then call ice_vsi_cfg() to > reconfigure the VSI as the same type that it was before. > > The existing reset logic assumes that all hardware filters will be removed, > so also call ice_fltr_remove_all() before re-configuring the VSI. > > This new operation does not re-create the VSI, so rename it to > ice_vf_reconfig_vsi(). > > The new approach can safely share the exact same flow for both SR-IOV VFs > as well as the Scalable IOV VFs being worked on. This uses less code and is > a better abstraction over fully deleting the VSI and adding a new one. > > Fixes: a4c785e8162e ("ice: convert vf_ops .vsi_rebuild to .create_vsi") > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > --- > Changes since v3: > * Removal of filters was a side effect of VSI being fully deleted by > firmware. Since we don't do that now, it was not happening. Fix this by > adding a call to ice_fltr_remove_all(). Thanks to Jakub Buchocki for > figuring out this solution. > * Rebase on to current dev-queue. > > v3 was posted at https://lore.kernel.org/intel-wired-lan/20230712222936.1048751-1-jacob.e.keller@intel.com/ > v2 was posted at https://lore.kernel.org/intel-wired-lan/20230712204730.326331-1-jacob.e.keller@intel.com/ > v1 was posted at https://lore.kernel.org/intel-wired-lan/20230504211829.3869868-1-jacob.e.keller@intel.com/ > > > drivers/net/ethernet/intel/ice/ice_sriov.c | 24 ++----------- > drivers/net/ethernet/intel/ice/ice_vf_lib.c | 35 +++++++++++++------ > drivers/net/ethernet/intel/ice/ice_vf_lib.h | 1 - > .../ethernet/intel/ice/ice_vf_lib_private.h | 1 + > 4 files changed, 28 insertions(+), 33 deletions(-) Issue from previous version is fixed. Thanks everything looks fine. Reviewed-by: Petr Oros <poros@redhat.com>
> -----Original Message----- > From: Petr Oros <poros@redhat.com> > Sent: Thursday, November 30, 2023 10:33 AM > To: Keller, Jacob E <jacob.e.keller@intel.com>; Intel Wired LAN <intel-wired- > lan@lists.osuosl.org> > Cc: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Romanowski, Rafal > <rafal.romanowski@intel.com>; Michal Swiatkowski > <michal.swiatkowski@linux.intel.com>; Buchocki, JakubX > <jakubx.buchocki@intel.com>; Nguyen, Anthony L > <anthony.l.nguyen@intel.com> > Subject: Re: [PATCH iwl-next v4] ice: replace ice_vf_recreate_vsi() with > ice_vf_reconfig_vsi() > > > Dne 28. 11. 23 v 20:42 Jacob Keller napsal(a): > > The ice_vf_create_vsi() function and its VF ops helper introduced by > > commit a4c785e8162e ("ice: convert vf_ops .vsi_rebuild to > > .create_vsi") are used during an individual VF reset to re-create the > > VSI. This was done in order to ensure that the VSI gets properly reconfigured > within the hardware. > > > > This is somewhat heavy handed as we completely release the VSI memory > > and structure, and then create a new VSI. This can also potentially > > force a change of the VSI index as we will re-use the first open slot > > in the VSI array which may not be the same. > > > > As part of implementing devlink reload, commit 6624e780a577 ("ice: > > split ice_vsi_setup into smaller functions") split VSI setup into > > smaller functions, introducing both ice_vsi_cfg() and ice_vsi_decfg() > > which can be used to configure or deconfigure an existing software VSI > structure. > > > > Rather than completely removing the VSI and adding a new one using the > > .create_vsi() VF operation, simply use ice_vsi_decfg() to remove the > > current configuration. Save the VSI type and then call ice_vsi_cfg() > > to reconfigure the VSI as the same type that it was before. > > > > The existing reset logic assumes that all hardware filters will be > > removed, so also call ice_fltr_remove_all() before re-configuring the VSI. > > > > This new operation does not re-create the VSI, so rename it to > > ice_vf_reconfig_vsi(). > > > > The new approach can safely share the exact same flow for both SR-IOV > > VFs as well as the Scalable IOV VFs being worked on. This uses less > > code and is a better abstraction over fully deleting the VSI and adding a new > one. > > > > Fixes: a4c785e8162e ("ice: convert vf_ops .vsi_rebuild to > > .create_vsi") > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > > --- > > Changes since v3: > > * Removal of filters was a side effect of VSI being fully deleted by > > firmware. Since we don't do that now, it was not happening. Fix this by > > adding a call to ice_fltr_remove_all(). Thanks to Jakub Buchocki for > > figuring out this solution. > > * Rebase on to current dev-queue. > > > > v3 was posted at > > https://lore.kernel.org/intel-wired-lan/20230712222936.1048751-1-jacob > > .e.keller@intel.com/ > > v2 was posted at > > https://lore.kernel.org/intel-wired-lan/20230712204730.326331-1-jacob. > > e.keller@intel.com/ > > v1 was posted at > > https://lore.kernel.org/intel-wired-lan/20230504211829.3869868-1-jacob > > .e.keller@intel.com/ > > > > > > drivers/net/ethernet/intel/ice/ice_sriov.c | 24 ++----------- > > drivers/net/ethernet/intel/ice/ice_vf_lib.c | 35 +++++++++++++------ > > drivers/net/ethernet/intel/ice/ice_vf_lib.h | 1 - > > .../ethernet/intel/ice/ice_vf_lib_private.h | 1 + > > 4 files changed, 28 insertions(+), 33 deletions(-) > Issue from previous version is fixed. Thanks > > everything looks fine. > > Reviewed-by: Petr Oros <poros@redhat.com> > Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c index 4ee349fe6409..a94a1c48c3de 100644 --- a/drivers/net/ethernet/intel/ice/ice_sriov.c +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c @@ -761,24 +761,6 @@ static void ice_sriov_clear_reset_trigger(struct ice_vf *vf) ice_flush(hw); } -/** - * ice_sriov_create_vsi - Create a new VSI for a VF - * @vf: VF to create the VSI for - * - * This is called by ice_vf_recreate_vsi to create the new VSI after the old - * VSI has been released. - */ -static int ice_sriov_create_vsi(struct ice_vf *vf) -{ - struct ice_vsi *vsi; - - vsi = ice_vf_vsi_setup(vf); - if (!vsi) - return -ENOMEM; - - return 0; -} - /** * ice_sriov_post_vsi_rebuild - tasks to do after the VF's VSI have been rebuilt * @vf: VF to perform tasks on @@ -798,7 +780,6 @@ static const struct ice_vf_ops ice_sriov_vf_ops = { .poll_reset_status = ice_sriov_poll_reset_status, .clear_reset_trigger = ice_sriov_clear_reset_trigger, .irq_close = NULL, - .create_vsi = ice_sriov_create_vsi, .post_vsi_rebuild = ice_sriov_post_vsi_rebuild, }; @@ -1141,8 +1122,7 @@ int ice_sriov_set_msix_vec_count(struct pci_dev *vf_dev, int msix_vec_count) if (vf->first_vector_idx < 0) goto unroll; - ice_vf_vsi_release(vf); - if (vf->vf_ops->create_vsi(vf)) { + if (ice_vf_reconfig_vsi(vf)) { /* Try to rebuild with previous values */ needs_rebuild = true; goto unroll; @@ -1169,7 +1149,7 @@ int ice_sriov_set_msix_vec_count(struct pci_dev *vf_dev, int msix_vec_count) return -EINVAL; if (needs_rebuild) - vf->vf_ops->create_vsi(vf); + ice_vf_reconfig_vsi(vf); ice_ena_vf_mappings(vf); ice_put_vf(vf); diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c index d6f74513b495..2ffdae9a82df 100644 --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c @@ -248,29 +248,44 @@ static void ice_vf_pre_vsi_rebuild(struct ice_vf *vf) } /** - * ice_vf_recreate_vsi - Release and re-create the VF's VSI - * @vf: VF to recreate the VSI for + * ice_vf_reconfig_vsi - Reconfigure a VF VSI with the device + * @vf: VF to reconfigure the VSI for * - * This is only called when a single VF is being reset (i.e. VVF, VFLR, host - * VF configuration change, etc) + * This is called when a single VF is being reset (i.e. VVF, VFLR, host VF + * configuration change, etc). * - * It releases and then re-creates a new VSI. + * It brings the VSI down and then reconfigures it with the hardware. */ -static int ice_vf_recreate_vsi(struct ice_vf *vf) +int ice_vf_reconfig_vsi(struct ice_vf *vf) { + struct ice_vsi *vsi = ice_get_vf_vsi(vf); + struct ice_vsi_cfg_params params = {}; struct ice_pf *pf = vf->pf; int err; - ice_vf_vsi_release(vf); + if (WARN_ON(!vsi)) + return -EINVAL; - err = vf->vf_ops->create_vsi(vf); + params = ice_vsi_to_params(vsi); + params.flags = ICE_VSI_FLAG_NO_INIT; + + ice_vsi_decfg(vsi); + ice_fltr_remove_all(vsi); + + err = ice_vsi_cfg(vsi, ¶ms); if (err) { dev_err(ice_pf_to_dev(pf), - "Failed to recreate the VF%u's VSI, error %d\n", + "Failed to reconfigure the VF%u's VSI, error %d\n", vf->vf_id, err); return err; } + /* Update the lan_vsi_num field since it might have been changed. The + * PF lan_vsi_idx number remains the same so we don't need to change + * that. + */ + vf->lan_vsi_num = vsi->vsi_num; + return 0; } @@ -928,7 +943,7 @@ int ice_reset_vf(struct ice_vf *vf, u32 flags) ice_vf_pre_vsi_rebuild(vf); - if (ice_vf_recreate_vsi(vf)) { + if (ice_vf_reconfig_vsi(vf)) { dev_err(dev, "Failed to release and setup the VF%u's VSI\n", vf->vf_id); err = -EFAULT; diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.h b/drivers/net/ethernet/intel/ice/ice_vf_lib.h index 35866553f288..0cc9034065c5 100644 --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.h +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.h @@ -62,7 +62,6 @@ struct ice_vf_ops { bool (*poll_reset_status)(struct ice_vf *vf); void (*clear_reset_trigger)(struct ice_vf *vf); void (*irq_close)(struct ice_vf *vf); - int (*create_vsi)(struct ice_vf *vf); void (*post_vsi_rebuild)(struct ice_vf *vf); }; diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib_private.h b/drivers/net/ethernet/intel/ice/ice_vf_lib_private.h index 0c7e77c0a09f..91ba7fe0eaee 100644 --- a/drivers/net/ethernet/intel/ice/ice_vf_lib_private.h +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib_private.h @@ -23,6 +23,7 @@ #warning "Only include ice_vf_lib_private.h in CONFIG_PCI_IOV virtualization files" #endif +int ice_vf_reconfig_vsi(struct ice_vf *vf); void ice_initialize_vf_entry(struct ice_vf *vf); void ice_dis_vf_qs(struct ice_vf *vf); int ice_check_vf_init(struct ice_vf *vf);
The ice_vf_create_vsi() function and its VF ops helper introduced by commit a4c785e8162e ("ice: convert vf_ops .vsi_rebuild to .create_vsi") are used during an individual VF reset to re-create the VSI. This was done in order to ensure that the VSI gets properly reconfigured within the hardware. This is somewhat heavy handed as we completely release the VSI memory and structure, and then create a new VSI. This can also potentially force a change of the VSI index as we will re-use the first open slot in the VSI array which may not be the same. As part of implementing devlink reload, commit 6624e780a577 ("ice: split ice_vsi_setup into smaller functions") split VSI setup into smaller functions, introducing both ice_vsi_cfg() and ice_vsi_decfg() which can be used to configure or deconfigure an existing software VSI structure. Rather than completely removing the VSI and adding a new one using the .create_vsi() VF operation, simply use ice_vsi_decfg() to remove the current configuration. Save the VSI type and then call ice_vsi_cfg() to reconfigure the VSI as the same type that it was before. The existing reset logic assumes that all hardware filters will be removed, so also call ice_fltr_remove_all() before re-configuring the VSI. This new operation does not re-create the VSI, so rename it to ice_vf_reconfig_vsi(). The new approach can safely share the exact same flow for both SR-IOV VFs as well as the Scalable IOV VFs being worked on. This uses less code and is a better abstraction over fully deleting the VSI and adding a new one. Fixes: a4c785e8162e ("ice: convert vf_ops .vsi_rebuild to .create_vsi") Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> --- Changes since v3: * Removal of filters was a side effect of VSI being fully deleted by firmware. Since we don't do that now, it was not happening. Fix this by adding a call to ice_fltr_remove_all(). Thanks to Jakub Buchocki for figuring out this solution. * Rebase on to current dev-queue. v3 was posted at https://lore.kernel.org/intel-wired-lan/20230712222936.1048751-1-jacob.e.keller@intel.com/ v2 was posted at https://lore.kernel.org/intel-wired-lan/20230712204730.326331-1-jacob.e.keller@intel.com/ v1 was posted at https://lore.kernel.org/intel-wired-lan/20230504211829.3869868-1-jacob.e.keller@intel.com/ drivers/net/ethernet/intel/ice/ice_sriov.c | 24 ++----------- drivers/net/ethernet/intel/ice/ice_vf_lib.c | 35 +++++++++++++------ drivers/net/ethernet/intel/ice/ice_vf_lib.h | 1 - .../ethernet/intel/ice/ice_vf_lib_private.h | 1 + 4 files changed, 28 insertions(+), 33 deletions(-) base-commit: 0392e4d0c1cd583c079a18685cd308f8ce903793