RE: [PATCH net 2/4] ice: Correctly handle aux device when num channels change

["Ertman, David M" <david.m.ertman@intel.com>]

> -----Original Message-----
> From: Saeed Mahameed <saeed@kernel.org>
> Sent: Wednesday, December 7, 2022 2:26 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> edumazet@google.com; Ertman, David M <david.m.ertman@intel.com>;
> netdev@vger.kernel.org; Saleem, Shiraz <shiraz.saleem@intel.com>; Ismail,
> Mustafa <mustafa.ismail@intel.com>; jgg@nvidia.com; leonro@nvidia.com;
> linux-rdma@vger.kernel.org; G, GurucharanX <gurucharanx.g@intel.com>
> Subject: Re: [PATCH net 2/4] ice: Correctly handle aux device when num
> channels change
> 
> On 07 Dec 13:10, Tony Nguyen wrote:
> >From: Dave Ertman <david.m.ertman@intel.com>
> >
> >When the number of channels/queues changes on an interface, it is
> necessary
> >to change how those resources are distributed to the auxiliary device for
> >maintaining RDMA functionality.  To do this, the best way is to unplug, and
> 
> Can you please explain how an ethtool can affect RDMA functionality ?
> don't you have full bifurcation between the two eth and rdma interfaces ..
> 
This patch is to address a bug where the number of queues for the interface was
changed and the RDMA lost functionality due to queues being re-assigned.

The PF is managing and setting aside resources for the RDMA aux dev. Then the 
RDMA aux driver will request resources from the PF driver.  Changes in
the total number of resources make it so that resources previously
allocated to RDMA aux driver may not be available any more.  A re-allocation
is necessary to ensure that RDMA has all of the queues that it thinks it does.

> >then re-plug the auxiliary device.  This will cause all current resource
> >allocation to be released, and then re-requested under the new state.
> >
> 
> I find this really disruptive, changing number of netdev queues to cause
> full aux devs restart !
> 

Changing the number of queues available to the interface *is* a disruptive action.
The netdev  and VSI have to be re-configured for queues per TC and the RDMA aux
driver has to re-allocate qsets to attach queue-pairs to.

> >Since the set_channel command from ethtool comes in while holding the
> RTNL
> >lock, it is necessary to offset the plugging and unplugging of auxiliary
> >device to another context.  For this purpose, set the flags for UNPLUG and
> >PLUG in the PF state, then respond to them in the service task.
> >
> >Also, since the auxiliary device will be unplugged/plugged at the end of
> >the flow, it is better to not send the event for TCs changing in the
> >middle of the flow.  This will prevent a timing issue between the events
> >and the probe/release calls conflicting.
> >
> >Fixes: 348048e724a0 ("ice: Implement iidc operations")
> >Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
> >Tested-by: Gurucharan G <gurucharanx.g@intel.com> (A Contingent
> worker at Intel)
> >Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> >---
> > drivers/net/ethernet/intel/ice/ice.h         | 2 ++
> > drivers/net/ethernet/intel/ice/ice_ethtool.c | 6 ++++++
> > drivers/net/ethernet/intel/ice/ice_idc.c     | 3 +++
> > drivers/net/ethernet/intel/ice/ice_main.c    | 3 +++
> > 4 files changed, 14 insertions(+)
> >
> >diff --git a/drivers/net/ethernet/intel/ice/ice.h
> b/drivers/net/ethernet/intel/ice/ice.h
> >index 001500afc4a6..092e572768fe 100644
> >--- a/drivers/net/ethernet/intel/ice/ice.h
> >+++ b/drivers/net/ethernet/intel/ice/ice.h
> >@@ -281,6 +281,7 @@ enum ice_pf_state {
> > 	ICE_FLTR_OVERFLOW_PROMISC,
> > 	ICE_VF_DIS,
> > 	ICE_CFG_BUSY,
> >+	ICE_SET_CHANNELS,
> > 	ICE_SERVICE_SCHED,
> > 	ICE_SERVICE_DIS,
> > 	ICE_FD_FLUSH_REQ,
> >@@ -485,6 +486,7 @@ enum ice_pf_flags {
> > 	ICE_FLAG_VF_VLAN_PRUNING,
> > 	ICE_FLAG_LINK_LENIENT_MODE_ENA,
> > 	ICE_FLAG_PLUG_AUX_DEV,
> >+	ICE_FLAG_UNPLUG_AUX_DEV,
> > 	ICE_FLAG_MTU_CHANGED,
> > 	ICE_FLAG_GNSS,			/* GNSS successfully
> initialized */
> > 	ICE_PF_FLAGS_NBITS		/* must be last */
> >diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> >index b7be84bbe72d..37e174a19860 100644
> >--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> >+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> >@@ -3536,6 +3536,8 @@ static int ice_set_channels(struct net_device
> *dev, struct ethtool_channels *ch)
> > 		return -EINVAL;
> > 	}
> >
> >+	set_bit(ICE_SET_CHANNELS, pf->state);
> >+
> > 	ice_vsi_recfg_qs(vsi, new_rx, new_tx);
> >
> > 	if (!netif_is_rxfh_configured(dev))
> >@@ -3543,6 +3545,10 @@ static int ice_set_channels(struct net_device
> *dev, struct ethtool_channels *ch)
> >
> > 	/* Update rss_size due to change in Rx queues */
> > 	vsi->rss_size = ice_get_valid_rss_size(&pf->hw, new_rx);
> >+	clear_bit(ICE_SET_CHANNELS, pf->state);
> >+
> 
> you just set this new state a few lines ago, clearing the bit in the same
> function few lines later seems to be an abuse of the pf state machine,
> couldn't you just pass a parameter to the functions which needed this
> information ?
> 

How is this abusing the PF state machine?  There is a 3 deep function call that needs
the information that this is a set_channel context, and each of those functions is called
from several locations - how is changing all of those functions to include a parameter
(that will be false for all of them but this instance) be less abusive than setting and
clearing a bit?

> >+	set_bit(ICE_FLAG_UNPLUG_AUX_DEV, pf->flags);
> >+	set_bit(ICE_FLAG_PLUG_AUX_DEV, pf->flags);
> >
> > 	return 0;
> > }