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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y5ES3kmYSINlAQhz@x130>
Date:   Wed, 7 Dec 2022 14:25:34 -0800
From:   Saeed Mahameed <saeed@...nel.org>
To:     Tony Nguyen <anthony.l.nguyen@...el.com>
Cc:     davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
        edumazet@...gle.com, Dave Ertman <david.m.ertman@...el.com>,
        netdev@...r.kernel.org, shiraz.saleem@...el.com,
        mustafa.ismail@...el.com, jgg@...dia.com, leonro@...dia.com,
        linux-rdma@...r.kernel.org, Gurucharan G <gurucharanx.g@...el.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@...el.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 .. 

>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 !

>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@...el.com>
>Tested-by: Gurucharan G <gurucharanx.g@...el.com> (A Contingent worker at Intel)
>Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.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 ? 

>+	set_bit(ICE_FLAG_UNPLUG_AUX_DEV, pf->flags);
>+	set_bit(ICE_FLAG_PLUG_AUX_DEV, pf->flags);
>
> 	return 0;
> }
>diff --git a/drivers/net/ethernet/intel/ice/ice_idc.c b/drivers/net/ethernet/intel/ice/ice_idc.c
>index 895c32bcc8b5..9bf6fa5ed4c8 100644
>--- a/drivers/net/ethernet/intel/ice/ice_idc.c
>+++ b/drivers/net/ethernet/intel/ice/ice_idc.c
>@@ -37,6 +37,9 @@ void ice_send_event_to_aux(struct ice_pf *pf, struct iidc_event *event)
> 	if (WARN_ON_ONCE(!in_task()))
> 		return;
>
>+	if (test_bit(ICE_SET_CHANNELS, pf->state))
>+		return;
>+
> 	mutex_lock(&pf->adev_mutex);
> 	if (!pf->adev)
> 		goto finish;
>diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
>index d0f14e73e8da..d58f55a72ab3 100644
>--- a/drivers/net/ethernet/intel/ice/ice_main.c
>+++ b/drivers/net/ethernet/intel/ice/ice_main.c
>@@ -2300,6 +2300,9 @@ static void ice_service_task(struct work_struct *work)
> 		}
> 	}
>
>+	if (test_and_clear_bit(ICE_FLAG_UNPLUG_AUX_DEV, pf->flags))
>+		ice_unplug_aux_dev(pf);
>+
> 	if (test_bit(ICE_FLAG_PLUG_AUX_DEV, pf->flags)) {
> 		/* Plug aux device per request */
> 		ice_plug_aux_dev(pf);
>-- 
>2.35.1
>