by Larry Finger

[permalink] [raw]

Subject: Re: [WIP] p54: deal with allocation failures in rx path

2009-07-06 13:16:53

by Christian Lamparter

[permalink] [raw]

Subject: Re: [WIP] p54: deal with allocation failures in rx path

On Monday 06 July 2009 03:36:59 Larry Finger wrote:
> Christian Lamparter wrote:
>
> > hmm, can you please give this a go? (I hope this patch still applies...)
> > I'm curious if you can dump the tx_queue when p54_alloc_skb fail?
>
> The patch applied with the aid of wiggle.
yeah, I don't want to delay these changes... so they are based on top of
the last _official_ patches, rather than the experimental ones.

> Since adding it, I've had 7 of the disassociation failures, but none of the
> queue len problems.
ohh, forgot to add that...
- do you think you can add another p54_dump_tx_queue
when it "disassociates". I'm curious if the queue was empty or is full
(with what?!)

> I have modified every place that p54_alloc_skb() fails with a printk
> that tells why it failed and then calls p54_dump_tx_queue(), but I
> have not gotten to test it yet. I found an oops when 2.6.31-rc2 is
> booted that I'm bisecting right now.
yeah, in the mean time I'll get these fixes to linville.

Regards,
Chr

2009-07-06 14:00:14

by Christian Lamparter

[permalink] [raw]

Subject: Re: [WIP] p54: deal with allocation failures in rx path

On Monday 06 July 2009 15:11:11 Max Filippov wrote:
> > This patch tries to address a long standing issue:
> > how to survive serve memory starvation situations,
> > without losing the device due to missing transfer-buffers.
>
> Christian, how does placement of skb allocation point make a
> difference?
not all drivers are equal... usb is the critical one.
just take a look at the these lines in p54u_rx_cb

skb = dev_alloc_skb(priv->common.rx_mtu + 32);
if (unlikely(!skb)) {
/* TODO check rx queue length and refill *somewhere* */
return;
}

so every time we failed to alloc the replacement skb, we have to
abandon one rx urb, until nothing is left and the user has
to replug/reload. pci is a bit more forgiving, but it can bite as well.
(Note: this scenario is _unlikely_ to happen ever since Larry scaled
down the rx_mtu from 3240 to about 2400 or so - which makes
dev_alloc_skb a 0-order allocation on the most common architectures)

> Anyway skb that carry data frame will be handed to the mac
> and replaced with new one. That is, this patch does not eliminate
> possibility of control frame loss due to insufficient memory.
control frames loss?
can you please point me exactly where?
Since this is a bug that needs to be fixed.

as control frames' skb should be (and hopefully are) always reused.
(see @ txrx.c line 590 - p54_rx_control -> return skb; )

> Maybe we'd better have separate reusable skb for control frames which
> is never handed out from the driver?
well as a matter of fact this is already true for p54pci & p54usb
(based on the assumption we are really recycling all control frames...
and not somehow forgot a case and got a leak there?!)

Regards,
Chr

2009-07-04 21:13:48

by Larry Finger

[permalink] [raw]

Subject: Re: [WIP] p54: deal with allocation failures in rx path

Larry Finger wrote:
> @@ -224,6 +236,7 @@ static void p54_tx_qos_accounting_free(s
> struct p54_tx_data *data = (void *) hdr->data;
>
> priv->tx_stats[data->hw_queue].len--;
> + WARN_ON(priv->tx_stats[data->hw_queue].len < 0);
> }
> p54_wake_queues(priv);
> }
>

The new WARN_ON did _NOT_ trigger when the len went negative.

The only other place where len could be decremented is through
txhdr->backlog. I noticed that the p54common.c had

txhdr->backlog = current_queue->len;

This was replaced in txrx.c by

txhdr->backlog = priv->tx_stats[queue].len - 1;

Was this intentional?

To test if this is the problem, I added the following hunk:

@@ -840,6 +853,7 @@ int p54_tx_80211(struct ieee80211_hw *de
txhdr->crypt_offset = crypt_offset;
txhdr->hw_queue = queue;
txhdr->backlog = priv->tx_stats[queue].len - 1;
+ WARN_ON(!priv->tx_stats[queue].len);
memset(txhdr->durations, 0, sizeof(txhdr->durations));
txhdr->tx_antenna = ((info->antenna_sel_tx == 0) ?
2 : info->antenna_sel_tx - 1) & priv->tx_diversity_mask;

This WARN_ON did trigger just before txq[6].len went negative. I'm now
testing with that changed as follows:

@@ -839,7 +852,8 @@ int p54_tx_80211(struct ieee80211_hw *de
}
txhdr->crypt_offset = crypt_offset;
txhdr->hw_queue = queue;
- txhdr->backlog = priv->tx_stats[queue].len - 1;
+ txhdr->backlog = priv->tx_stats[queue].len;
+ WARN_ON(priv->tx_stats[queue].len < 0);
memset(txhdr->durations, 0, sizeof(txhdr->durations));
txhdr->tx_antenna = ((info->antenna_sel_tx == 0) ?
2 : info->antenna_sel_tx - 1) & priv->tx_diversity_mask;

This WARN_ON did not trigger, but I still had the queue len go negative.

One other question: struct p54_burst is defined in lmac.h, but it
doesn't seem to be used anywhere. Will it be needed later?

Larry

2009-07-04 07:53:00

> > hmm, looks like someone tries to skb_push on a NULL skb. hmmmm,
> > can you please enable ksym, it's a bit hard to see the obvious bug here.

> [ 1612.708465] skb_over_panic: text:bf000544 len:88 put:88 head:c78f4000 data:c78f4020 tail:0xc78f4078 end:0xc78f4020 dev:<NULL>

I see here valid skb with size 0x20, where we try to put 88 more bytes.
That's because p54spi_probe calls __dev_alloc_skb before p54spi_request_firmware.

It's working with the following change:

diff --git a/drivers/net/wireless/p54/p54spi.c b/drivers/net/wireless/p54/p54spi.c
index ab5b9b8..ff73a64 100644
--- a/drivers/net/wireless/p54/p54spi.c
+++ b/drivers/net/wireless/p54/p54spi.c
@@ -651,11 +651,6 @@ static int __devinit p54spi_probe(struct spi_device *spi)
priv->common.stop = p54spi_op_stop;
priv->common.tx = p54spi_op_tx;

- skb = __dev_alloc_skb(priv->common.rx_mtu, GFP_KERNEL);
- if (!skb)
- goto err_free_common;
- skb_queue_tail(&priv->rx_pool, skb);
-
ret = p54spi_request_firmware(hw);
if (ret < 0)
goto err_free_common;
@@ -664,6 +659,11 @@ static int __devinit p54spi_probe(struct spi_device *spi)
if (ret)
goto err_free_common;

+ skb = __dev_alloc_skb(priv->common.rx_mtu, GFP_KERNEL);
+ if (!skb)
+ goto err_free_common;
+ skb_queue_tail(&priv->rx_pool, skb);
+
ret = p54_register_common(hw, &priv->spi->dev);
if (ret)
goto err_free_common;

Still cannot stress-test it: it hangs in IBSS mode (I suspect rate control)
and it cannot initialize mesh: firmware doesn't respond after beacon submission.

Does mesh work now with USB/PCI?

Thanks.
-- Max

2009-07-06 13:11:09

by Max Filippov

[permalink] [raw]

Subject: Re: [WIP] p54: deal with allocation failures in rx path

> This patch tries to address a long standing issue:
> how to survive serve memory starvation situations,
> without losing the device due to missing transfer-buffers.

Christian, how does placement of skb allocation point make a
difference? Anyway skb that carry data frame will be handed to the mac
and replaced with new one. That is, this patch does not eliminate
possibility of control frame loss due to insufficient memory.
Maybe we'd better have separate reusable skb for control frames which
is never handed out from the driver?

--
Max