From: Saeed Mahameed <saeed@kernel.org>
To: Tony Nguyen <anthony.l.nguyen@intel.com>
Cc: davem@davemloft.net, kuba@kernel.org, pabeni@redhat.com,
edumazet@google.com, Dave Ertman <david.m.ertman@intel.com>,
netdev@vger.kernel.org, shiraz.saleem@intel.com,
mustafa.ismail@intel.com, jgg@nvidia.com, leonro@nvidia.com,
linux-rdma@vger.kernel.org,
Gurucharan G <gurucharanx.g@intel.com>
Subject: Re: [PATCH net 2/4] ice: Correctly handle aux device when num channels change
Date: Wed, 7 Dec 2022 14:25:34 -0800 [thread overview]
Message-ID: <Y5ES3kmYSINlAQhz@x130> (raw)
In-Reply-To: <20221207211040.1099708-3-anthony.l.nguyen@intel.com>
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 ..
>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@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 ?
>+ 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
>
next prev parent reply other threads:[~2022-12-07 22:25 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20221207211040.1099708-1-anthony.l.nguyen@intel.com>
2022-12-07 21:10 ` [PATCH net 2/4] ice: Correctly handle aux device when num channels change Tony Nguyen
2022-12-07 22:25 ` Saeed Mahameed [this message]
2022-12-09 17:21 ` Ertman, David M
2022-12-09 19:28 ` Saeed Mahameed
2022-12-09 19:32 ` Jason Gunthorpe
2022-12-12 17:03 ` Ertman, David M
2022-12-12 23:53 ` Jason Gunthorpe
2022-12-16 19:08 ` [PATCH net 2/4] ice: git send-email --suppress-cc=all --to e1000-patches@eclists.intel.com Ertman, David M
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y5ES3kmYSINlAQhz@x130 \
--to=saeed@kernel.org \
--cc=anthony.l.nguyen@intel.com \
--cc=davem@davemloft.net \
--cc=david.m.ertman@intel.com \
--cc=edumazet@google.com \
--cc=gurucharanx.g@intel.com \
--cc=jgg@nvidia.com \
--cc=kuba@kernel.org \
--cc=leonro@nvidia.com \
--cc=linux-rdma@vger.kernel.org \
--cc=mustafa.ismail@intel.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=shiraz.saleem@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).