* [PATCH net] ionic: centralize queue reset code
@ 2020-07-02 23:39 Shannon Nelson
2020-07-06 17:33 ` Jakub Kicinski
0 siblings, 1 reply; 5+ messages in thread
From: Shannon Nelson @ 2020-07-02 23:39 UTC (permalink / raw)
To: netdev, davem; +Cc: Shannon Nelson
The queue reset pattern is used in a couple different places,
only slightly different from each other, and could cause
issues if one gets changed and the other didn't. This puts
them together so that only one version is needed, yet each
can have slighty different effects by passing in a pointer
to a work function to do whatever configuration twiddling is
needed in the middle of the reset.
Fixes: 4d03e00a2140 ("ionic: Add initial ethtool support")
Signed-off-by: Shannon Nelson <snelson@pensando.io>
---
.../ethernet/pensando/ionic/ionic_ethtool.c | 52 ++++++-------------
.../net/ethernet/pensando/ionic/ionic_lif.c | 17 ++++--
.../net/ethernet/pensando/ionic/ionic_lif.h | 4 +-
3 files changed, 32 insertions(+), 41 deletions(-)
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
index f7e3ce3de04d..e03ea9b18f95 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
@@ -468,12 +468,18 @@ static void ionic_get_ringparam(struct net_device *netdev,
ring->rx_pending = lif->nrxq_descs;
}
+static void ionic_set_ringsize(struct ionic_lif *lif, void *arg)
+{
+ struct ethtool_ringparam *ring = arg;
+
+ lif->ntxq_descs = ring->tx_pending;
+ lif->nrxq_descs = ring->rx_pending;
+}
+
static int ionic_set_ringparam(struct net_device *netdev,
struct ethtool_ringparam *ring)
{
struct ionic_lif *lif = netdev_priv(netdev);
- bool running;
- int err;
if (ring->rx_mini_pending || ring->rx_jumbo_pending) {
netdev_info(netdev, "Changing jumbo or mini descriptors not supported\n");
@@ -491,22 +497,7 @@ static int ionic_set_ringparam(struct net_device *netdev,
ring->rx_pending == lif->nrxq_descs)
return 0;
- err = ionic_wait_for_bit(lif, IONIC_LIF_F_QUEUE_RESET);
- if (err)
- return err;
-
- running = test_bit(IONIC_LIF_F_UP, lif->state);
- if (running)
- ionic_stop(netdev);
-
- lif->ntxq_descs = ring->tx_pending;
- lif->nrxq_descs = ring->rx_pending;
-
- if (running)
- ionic_open(netdev);
- clear_bit(IONIC_LIF_F_QUEUE_RESET, lif->state);
-
- return 0;
+ return ionic_reset_queues(lif, ionic_set_ringsize, ring);
}
static void ionic_get_channels(struct net_device *netdev,
@@ -521,12 +512,17 @@ static void ionic_get_channels(struct net_device *netdev,
ch->combined_count = lif->nxqs;
}
+static void ionic_set_queuecount(struct ionic_lif *lif, void *arg)
+{
+ struct ethtool_channels *ch = arg;
+
+ lif->nxqs = ch->combined_count;
+}
+
static int ionic_set_channels(struct net_device *netdev,
struct ethtool_channels *ch)
{
struct ionic_lif *lif = netdev_priv(netdev);
- bool running;
- int err;
if (!ch->combined_count || ch->other_count ||
ch->rx_count || ch->tx_count)
@@ -535,21 +531,7 @@ static int ionic_set_channels(struct net_device *netdev,
if (ch->combined_count == lif->nxqs)
return 0;
- err = ionic_wait_for_bit(lif, IONIC_LIF_F_QUEUE_RESET);
- if (err)
- return err;
-
- running = test_bit(IONIC_LIF_F_UP, lif->state);
- if (running)
- ionic_stop(netdev);
-
- lif->nxqs = ch->combined_count;
-
- if (running)
- ionic_open(netdev);
- clear_bit(IONIC_LIF_F_QUEUE_RESET, lif->state);
-
- return 0;
+ return ionic_reset_queues(lif, ionic_set_queuecount, ch);
}
static u32 ionic_get_priv_flags(struct net_device *netdev)
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
index 3c9dde31f3fa..f49486b6d04d 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
@@ -1313,7 +1313,7 @@ static int ionic_change_mtu(struct net_device *netdev, int new_mtu)
return err;
netdev->mtu = new_mtu;
- err = ionic_reset_queues(lif);
+ err = ionic_reset_queues(lif, NULL, NULL);
return err;
}
@@ -1325,7 +1325,7 @@ static void ionic_tx_timeout_work(struct work_struct *ws)
netdev_info(lif->netdev, "Tx Timeout recovery\n");
rtnl_lock();
- ionic_reset_queues(lif);
+ ionic_reset_queues(lif, NULL, NULL);
rtnl_unlock();
}
@@ -1988,7 +1988,7 @@ static const struct net_device_ops ionic_netdev_ops = {
.ndo_get_vf_stats = ionic_get_vf_stats,
};
-int ionic_reset_queues(struct ionic_lif *lif)
+int ionic_reset_queues(struct ionic_lif *lif, ionic_reset_cb cb, void *arg)
{
bool running;
int err = 0;
@@ -2001,12 +2001,19 @@ int ionic_reset_queues(struct ionic_lif *lif)
if (running) {
netif_device_detach(lif->netdev);
err = ionic_stop(lif->netdev);
+ if (err)
+ goto reset_out;
}
- if (!err && running) {
- ionic_open(lif->netdev);
+
+ if (cb)
+ cb(lif, arg);
+
+ if (running) {
+ err = ionic_open(lif->netdev);
netif_device_attach(lif->netdev);
}
+reset_out:
clear_bit(IONIC_LIF_F_QUEUE_RESET, lif->state);
return err;
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
index c3428034a17b..ed126dd74e01 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
@@ -248,6 +248,8 @@ static inline u32 ionic_coal_hw_to_usec(struct ionic *ionic, u32 units)
return (units * div) / mult;
}
+typedef void (*ionic_reset_cb)(struct ionic_lif *lif, void *arg);
+
void ionic_link_status_check_request(struct ionic_lif *lif);
void ionic_get_stats64(struct net_device *netdev,
struct rtnl_link_stats64 *ns);
@@ -267,7 +269,7 @@ int ionic_lif_rss_config(struct ionic_lif *lif, u16 types,
int ionic_open(struct net_device *netdev);
int ionic_stop(struct net_device *netdev);
-int ionic_reset_queues(struct ionic_lif *lif);
+int ionic_reset_queues(struct ionic_lif *lif, ionic_reset_cb cb, void *arg);
static inline void debug_stats_txq_post(struct ionic_qcq *qcq,
struct ionic_txq_desc *desc, bool dbell)
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net] ionic: centralize queue reset code
2020-07-02 23:39 [PATCH net] ionic: centralize queue reset code Shannon Nelson
@ 2020-07-06 17:33 ` Jakub Kicinski
2020-07-07 16:10 ` Shannon Nelson
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Kicinski @ 2020-07-06 17:33 UTC (permalink / raw)
To: Shannon Nelson; +Cc: netdev, davem
On Thu, 2 Jul 2020 16:39:17 -0700 Shannon Nelson wrote:
> The queue reset pattern is used in a couple different places,
> only slightly different from each other, and could cause
> issues if one gets changed and the other didn't. This puts
> them together so that only one version is needed, yet each
> can have slighty different effects by passing in a pointer
> to a work function to do whatever configuration twiddling is
> needed in the middle of the reset.
>
> Fixes: 4d03e00a2140 ("ionic: Add initial ethtool support")
> Signed-off-by: Shannon Nelson <snelson@pensando.io>
Is this fixing anything?
I think the pattern of having a separate structure describing all the
parameters and passing that into reconfig is a better path forward,
because it's easier to take that forward in the correct direction of
allocating new resources before old ones are freed. IOW not doing a
full close/open.
E.g. nfp_net_set_ring_size().
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] ionic: centralize queue reset code
2020-07-06 17:33 ` Jakub Kicinski
@ 2020-07-07 16:10 ` Shannon Nelson
2020-07-07 16:46 ` Jakub Kicinski
0 siblings, 1 reply; 5+ messages in thread
From: Shannon Nelson @ 2020-07-07 16:10 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, davem
On 7/6/20 10:33 AM, Jakub Kicinski wrote:
> On Thu, 2 Jul 2020 16:39:17 -0700 Shannon Nelson wrote:
>> The queue reset pattern is used in a couple different places,
>> only slightly different from each other, and could cause
>> issues if one gets changed and the other didn't. This puts
>> them together so that only one version is needed, yet each
>> can have slighty different effects by passing in a pointer
>> to a work function to do whatever configuration twiddling is
>> needed in the middle of the reset.
>>
>> Fixes: 4d03e00a2140 ("ionic: Add initial ethtool support")
>> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> Is this fixing anything?
Yes, this fixes issues seen similar to what was fixed with b59eabd23ee5
("ionic: tame the watchdog timer on reconfig") where under loops of
changing parameters we could occasionally bump into the netdev watchdog.
>
> I think the pattern of having a separate structure describing all the
> parameters and passing that into reconfig is a better path forward,
> because it's easier to take that forward in the correct direction of
> allocating new resources before old ones are freed. IOW not doing a
> full close/open.
>
> E.g. nfp_net_set_ring_size().
This has been suggested before and looks great when you know you've got
the resources for dual allocations. In our case this code is also used
inside our device where memory is tight: we are much more likely to have
allocation issues if we try to allocate everything without first
releasing what we already have.
I agree there is room for evolution, and we have patches coming that
change some of how we allocate our memory, but we're not quite ready to
rewrite what we have, or to split the two driver cases yet.
sln
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] ionic: centralize queue reset code
2020-07-07 16:10 ` Shannon Nelson
@ 2020-07-07 16:46 ` Jakub Kicinski
2020-07-07 18:04 ` Shannon Nelson
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Kicinski @ 2020-07-07 16:46 UTC (permalink / raw)
To: Shannon Nelson; +Cc: netdev, davem
On Tue, 7 Jul 2020 09:10:38 -0700 Shannon Nelson wrote:
> On 7/6/20 10:33 AM, Jakub Kicinski wrote:
> > On Thu, 2 Jul 2020 16:39:17 -0700 Shannon Nelson wrote:
> >> The queue reset pattern is used in a couple different places,
> >> only slightly different from each other, and could cause
> >> issues if one gets changed and the other didn't. This puts
> >> them together so that only one version is needed, yet each
> >> can have slighty different effects by passing in a pointer
> >> to a work function to do whatever configuration twiddling is
> >> needed in the middle of the reset.
> >>
> >> Fixes: 4d03e00a2140 ("ionic: Add initial ethtool support")
> >> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> > Is this fixing anything?
>
> Yes, this fixes issues seen similar to what was fixed with b59eabd23ee5
> ("ionic: tame the watchdog timer on reconfig") where under loops of
> changing parameters we could occasionally bump into the netdev watchdog.
User-visible bug should always be part of the commit message for a fix,
please amend.
> > I think the pattern of having a separate structure describing all the
> > parameters and passing that into reconfig is a better path forward,
> > because it's easier to take that forward in the correct direction of
> > allocating new resources before old ones are freed. IOW not doing a
> > full close/open.
> >
> > E.g. nfp_net_set_ring_size().
>
> This has been suggested before and looks great when you know you've got
> the resources for dual allocations. In our case this code is also used
> inside our device where memory is tight: we are much more likely to have
> allocation issues if we try to allocate everything without first
> releasing what we already have.
Are you saying that inside the device the memory allocated for the
rings is close to the amount of max free memory? I find that hard to
believe.
> I agree there is room for evolution, and we have patches coming that
> change some of how we allocate our memory, but we're not quite ready to
> rewrite what we have, or to split the two driver cases yet.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] ionic: centralize queue reset code
2020-07-07 16:46 ` Jakub Kicinski
@ 2020-07-07 18:04 ` Shannon Nelson
0 siblings, 0 replies; 5+ messages in thread
From: Shannon Nelson @ 2020-07-07 18:04 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, davem
On 7/7/20 9:46 AM, Jakub Kicinski wrote:
> On Tue, 7 Jul 2020 09:10:38 -0700 Shannon Nelson wrote:
>> On 7/6/20 10:33 AM, Jakub Kicinski wrote:
>>> On Thu, 2 Jul 2020 16:39:17 -0700 Shannon Nelson wrote:
>>>> The queue reset pattern is used in a couple different places,
>>>> only slightly different from each other, and could cause
>>>> issues if one gets changed and the other didn't. This puts
>>>> them together so that only one version is needed, yet each
>>>> can have slighty different effects by passing in a pointer
>>>> to a work function to do whatever configuration twiddling is
>>>> needed in the middle of the reset.
>>>>
>>>> Fixes: 4d03e00a2140 ("ionic: Add initial ethtool support")
>>>> Signed-off-by: Shannon Nelson <snelson@pensando.io>
>>> Is this fixing anything?
>> Yes, this fixes issues seen similar to what was fixed with b59eabd23ee5
>> ("ionic: tame the watchdog timer on reconfig") where under loops of
>> changing parameters we could occasionally bump into the netdev watchdog.
> User-visible bug should always be part of the commit message for a fix,
> please amend.
Sure, I'll follow up later today.
>
>>> I think the pattern of having a separate structure describing all the
>>> parameters and passing that into reconfig is a better path forward,
>>> because it's easier to take that forward in the correct direction of
>>> allocating new resources before old ones are freed. IOW not doing a
>>> full close/open.
>>>
>>> E.g. nfp_net_set_ring_size().
>> This has been suggested before and looks great when you know you've got
>> the resources for dual allocations. In our case this code is also used
>> inside our device where memory is tight: we are much more likely to have
>> allocation issues if we try to allocate everything without first
>> releasing what we already have.
> Are you saying that inside the device the memory allocated for the
> rings is close to the amount of max free memory? I find that hard to
> believe.
>
I was a bit surprised as well, but there is a lot going on inside the
device and it turns out there are one or two specific use cases where
this is an issue.
sln
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-07-07 18:04 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-02 23:39 [PATCH net] ionic: centralize queue reset code Shannon Nelson
2020-07-06 17:33 ` Jakub Kicinski
2020-07-07 16:10 ` Shannon Nelson
2020-07-07 16:46 ` Jakub Kicinski
2020-07-07 18:04 ` Shannon Nelson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.