Re: [Intel-wired-lan] [PATCH net v1] ice: Ignore RDMA message on setup tc qdisc

[Michal Swiatkowski <michal.swiatkowski@linux.intel.com>]

On Thu, Jan 12, 2023 at 01:54:00PM +0000, Rogalski, RafalX wrote:
> >-----Original Message-----
> >From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> 
> >Sent: Thursday, January 12, 2023 8:12 AM
> >To: Maziarz, Kamil <kamil.maziarz@intel.com>
> >Cc: intel-wired-lan@lists.osuosl.org; Rogalski, RafalX <rafalx.rogalski@intel.com>
> >Subject: Re: [Intel-wired-lan] [PATCH net v1] ice: Ignore RDMA message on setup tc qdisc
> >
> >On Wed, Jan 11, 2023 at 03:20:29PM +0100, Kamil Maziarz wrote:
> >> From: Rafal Rogalski <rafalx.rogalski@intel.com>
> >> 
> >> Commit f70b9d5f4426 ("ice: check for a leaf node presence") prevents 
> >> removal of VSI with leaf nodes. This is an expectation of driver 
> >> action induced by FW requirements. However, this caused RDMA scheduler 
> >> config removal to fail every time a qdisc was added or deleted.
> >> 
> >> Fix this by ignoring errors from RDMA configuration removal when qdisc 
> >> are being reconfigured.
> >> 
> >> Fixes: ff7e93219442 ("ice: Fix failure to re-add LAN/RDMA Tx queues")
> >> Signed-off-by: Rafal Rogalski <rafalx.rogalski@intel.com>
> >> Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>
> >> ---
> >Thanks for changes. It is a good practise to include changelog here.
> >Something like:
> >v1 --> v2:
> > - check for -EBUSY etc.
> > ---
> >
> > Is Your patch rebased on dev-queue? I am pretty sure that  ice_vsi_rebuild looks different on current dev-queue, please check  that.
> >
> Thank you for vigilance. Yes, my changes are pushed at the top of dev-queue branch to the net-queue repository.
> 
Yeah, sorry for confusion, didn't notice that it is a fix to net. Thanks
for clarification.

> >>  drivers/net/ethernet/intel/ice/ice.h      | 1 +
> >>  drivers/net/ethernet/intel/ice/ice_lib.c  | 2 +-  
> >> drivers/net/ethernet/intel/ice/ice_main.c | 3 +++
> >>  3 files changed, 5 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/net/ethernet/intel/ice/ice.h 
> >> b/drivers/net/ethernet/intel/ice/ice.h
> >> index 2f0b604abc5e..b572d07bc126 100644
> >> --- a/drivers/net/ethernet/intel/ice/ice.h
> >> +++ b/drivers/net/ethernet/intel/ice/ice.h
> >> @@ -306,6 +306,7 @@ enum ice_pf_state {
> >>  	ICE_PHY_INIT_COMPLETE,
> >>  	ICE_FD_VF_FLUSH_CTX,		/* set at FD Rx IRQ or timeout */
> >>  	ICE_AUX_ERR_PENDING,
> >> +	ICE_SETUP_TC_QDISC,
> >>  	ICE_STATE_NBITS		/* must be last */
> >>  };
> >>  
> >> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c 
> >> b/drivers/net/ethernet/intel/ice/ice_lib.c
> >> index 22bcb414546a..0ee3acbea108 100644
> >> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> >> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> >> @@ -3448,7 +3448,7 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, bool 
> >> init_vsi)
> >>  
> >>  	ice_rm_vsi_lan_cfg(vsi->port_info, vsi->idx);
> >>  	ret = ice_rm_vsi_rdma_cfg(vsi->port_info, vsi->idx);
> >> -	if (ret)
> >> +	if (ret && !(test_bit(ICE_SETUP_TC_QDISC, pf->state) && ret == 
> >> +-EBUSY))
> >It is fine for me in current state, but do we really need to do this check only in setting TC qdisc? Doesn't the same thing happen when the number of queues is changed from ethtool? (maybe it is not because the changes is blocked when RDMA is active). Maybe whole
> >ice_setup_tc_mqprio_qdisc() should return -EBOUSY or inval if RDMA is active? Is it valid to a new qdisc when RDMA is active?
> >
> You are right about ethtool. However, I'm not sure if we should ignore -EBOUSY for every other scenario.
> 
Dave recently added a patchset thath blocks any queue changes when RDMA
driver is bound to aux [1]. I think following the same logic in
ice_setup_tc_mqprio_qdisc() the same check should happen (because it is
changing queues).

Adding Dave

Thanks, Michal
> >>  		dev_err(ice_pf_to_dev(vsi->back), "Failed to remove RDMA scheduler config for VSI %u, err %d\n",
> >>  			vsi->vsi_num, ret);
> >>  	ice_vsi_free_q_vectors(vsi);
> >> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c 
> >> b/drivers/net/ethernet/intel/ice/ice_main.c
> >> index a9a7f8b52140..5ff137645f08 100644
> >> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> >> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> >> @@ -8706,6 +8706,7 @@ static int ice_setup_tc_mqprio_qdisc(struct net_device *netdev, void *type_data)
> >>  	cur_txq = vsi->num_txq;
> >>  	cur_rxq = vsi->num_rxq;
> >>  
> >> +	set_bit(ICE_SETUP_TC_QDISC, pf->state);
> >>  	/* proceed with rebuild main VSI using correct number of queues */
> >>  	ret = ice_vsi_rebuild(vsi, false);
> >>  	if (ret) {
> >> @@ -8716,9 +8717,11 @@ static int ice_setup_tc_mqprio_qdisc(struct net_device *netdev, void *type_data)
> >>  		clear_bit(ICE_RESET_FAILED, pf->state);
> >>  		if (ice_vsi_rebuild(vsi, false)) {
> >>  			dev_err(dev, "Rebuild of main VSI failed again\n");
> >> +			clear_bit(ICE_SETUP_TC_QDISC, pf->state);
> >>  			return ret;
> >>  		}
> >>  	}
> >> +	clear_bit(ICE_SETUP_TC_QDISC, pf->state);
> >>  
> >>  	vsi->all_numtc = num_tcf;
> >>  	vsi->all_enatc = ena_tc_qdisc;
> >> --
> >> 2.31.1
> >> 
> >> _______________________________________________
> >> Intel-wired-lan mailing list
> >> Intel-wired-lan@osuosl.org
> >> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan