diff mbox

solos-pci: don't call vcc->pop() after pclose()

Message ID 20121129124344.GA7704@shrek.podlesie.net
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Krzysztof Mazur Nov. 29, 2012, 12:43 p.m. UTC 
On Thu, Nov 29, 2012 at 11:55:43AM +0000, David Woodhouse wrote:
> On Thu, 2012-11-29 at 11:57 +0100, Krzysztof Mazur wrote:
> > do we really need to wait here?
> > Why don't just do something like that:
> > 
> > 	tasklet_disable(&card->tlet);
> > 	spin_lock(&card->tx_queue_lock);
> > 	for each skb in queue
> > 		SKB_CB(skb)->vcc = NULL;
> > 	spin_unlock(&card->tx_queue_lock);
> > 	tasklet_enable(&card->tlet);
> > 
> > or if we really want to call vcc->pop() for such skbs:
> > 
> > 	tasklet_disable(&card->tlet);
> > 	spin_lock(&card->tx_queue_lock);
> > 	for each skb in queue {
> > 		skb_get(skb);
> > 		solos_pop(SKB_CB(skb)->vcc, skb);
> > 		SKB_CB(skb)->vcc = NULL;
> > 	}
> > 	spin_unlock(&card->tx_queue_lock);
> > 	tasklet_enable(&card->tlet);
> 
> Yes, we could certainly remove the packets from the tx_queue first.
> 
> However, in the card->using_dma case there might be a skb for this vcc
> *currently* being DMA'd, and we'd still need to wait for that one.

Removing packets from tx_queue is not needed. We can transmit packets
also after close. We just can't call vcc->pop() after close,
so we can just set SKB_CB(skb)->vcc of such packets to NULL so fpga_tx()
won't call vcc->pop().

Maybe I was not precise enough, I'm think that all we need is
something like that:

-- >8 --
Subject: [PATCH] solos-pci: don't call vcc->pop() after pclose()

After atmdev_ops->close() we cannot use vcc->pop() because the vcc may,
and probably will be destroyed.

We can just set vcc for such frames to NULL because fpga_tx() after
completion will call dev_kfree_skb() in that case.

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
---
 drivers/atm/solos-pci.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

David Woodhouse Nov. 29, 2012, 12:57 p.m. UTC | #1 
On Thu, 2012-11-29 at 13:43 +0100, Krzysztof Mazur wrote:
> 
> Removing packets from tx_queue is not needed. We can transmit packets
> also after close. We just can't call vcc->pop() after close,
> so we can just set SKB_CB(skb)->vcc of such packets to NULL so
> fpga_tx() won't call vcc->pop().

Your patch doesn't do that, does it? You'd want something like

 if (card->tx_skb[port] && SKB_CB(card->tx_skb[port]->vcc) == vcc)
    SKB_CB(card->tx_skb[port]->vcc) = NULL;

Under card->tx_lock should suffice.

And do we just *not* call the ->pop() on that skb ever? And hope that it
doesn't screw up some other state somewhere? Like if we're doing MLPPP
and I've implemented BQL for PPP... we might never call
ppp_completed_queue() for that skb, so even though this *channel* is
going away, it might still contribute towards the perceived queue on the
overall PPP netdev?

Failing to call ->pop() could cause memory leaks and other issues; I
don't think it's reasonable. I think we *have* to wait for
card->tx_skb[port] if it's for the VCC we're closing.
Krzysztof Mazur Nov. 29, 2012, 1:20 p.m. UTC | #2 
On Thu, Nov 29, 2012 at 12:57:17PM +0000, David Woodhouse wrote:
> On Thu, 2012-11-29 at 13:43 +0100, Krzysztof Mazur wrote:
> > 
> > Removing packets from tx_queue is not needed. We can transmit packets
> > also after close. We just can't call vcc->pop() after close,
> > so we can just set SKB_CB(skb)->vcc of such packets to NULL so
> > fpga_tx() won't call vcc->pop().
> 
> Your patch doesn't do that, does it? You'd want something like
> 
>  if (card->tx_skb[port] && SKB_CB(card->tx_skb[port]->vcc) == vcc)
>     SKB_CB(card->tx_skb[port]->vcc) = NULL;

No, I want to process all queued packets, not just only those that
were already sent do card.

In that case we will need to remove other packets with that vcc
from queue, of couse we can still do that in the same loop, something
like:

	if (SKB_CB(skb)->vcc == vcc) {
		if (card->tx_skb[port] == skb) {
			skb_get(skb);
			solos_pop(SKB_CB(skb)->vcc, skb);
			SKB_CB(skb)->vcc = NULL;
		} else {
			skb_unlink(skb, &card->tx_queue[port]);
			solos_pop(SKB_CB(skb)->vcc, skb);
		}
	}

But I don't think that this optization is needed.

> 
> Under card->tx_lock should suffice.
> 
> And do we just *not* call the ->pop() on that skb ever? And hope that it
> doesn't screw up some other state somewhere? Like if we're doing MLPPP
> and I've implemented BQL for PPP... we might never call
> ppp_completed_queue() for that skb, so even though this *channel* is
> going away, it might still contribute towards the perceived queue on the
> overall PPP netdev?
> 
> Failing to call ->pop() could cause memory leaks and other issues; I
> don't think it's reasonable. I think we *have* to wait for
> card->tx_skb[port] if it's for the VCC we're closing.

We are calling ->pop() in solos_pop() just before SKB_CB(skb)->vcc = NULL,
but we are doing that before we really finish processing that packet,
that's why we do skb_get(skb).

Krzysiek
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
chas williams - CONTRACTOR Nov. 29, 2012, 2:41 p.m. UTC | #3 
On Thu, 29 Nov 2012 13:43:44 +0100
Krzysztof Mazur <krzysiek@podlesie.net> wrote:

> Removing packets from tx_queue is not needed. We can transmit packets
> also after close. We just can't call vcc->pop() after close,
> so we can just set SKB_CB(skb)->vcc of such packets to NULL so fpga_tx()
> won't call vcc->pop().

i dont think you can transmit packets after close().  you can transmit
packets during close() though.  if you transmit after close that means
that you are using the vpi/vci pair that the atm stack thinks is no
longer in use.  additionally after close(), the hardware should be in a
state such that you cannot transmit or receive on the vpi/vci that has
been closed.

close() needs to make sure that any pending tx packets are sent or
otherwise disposed of (like turning off the transmit segmentation
engine, clearing the packets, or whatever).  any partially reassembled
pdu's also need to be cleared as well.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Woodhouse Nov. 29, 2012, 2:42 p.m. UTC | #4 
On Thu, 2012-11-29 at 14:20 +0100, Krzysztof Mazur wrote:
>                 if (card->tx_skb[port] == skb) {
>                         skb_get(skb);
>                         solos_pop(SKB_CB(skb)->vcc, skb);
>                         SKB_CB(skb)->vcc = NULL;

Um... yes, that would probably work. But it's subtle enough that it
bothers me. And if it *did* cause any strange issues, it's a rare case
and would be hard to reproduce/debug. Is it *really* necessary, just to
speed up the vcc close? I'm inclined to stick with the 'KISS' approach.

(Note that the card->tx_skb[port] skb isn't on the queue any more; you'd
do that bit separately and not in the skb_queue_walk() loop.)
Krzysztof Mazur Nov. 29, 2012, 2:55 p.m. UTC | #5 
On Thu, Nov 29, 2012 at 02:42:17PM +0000, David Woodhouse wrote:
> On Thu, 2012-11-29 at 14:20 +0100, Krzysztof Mazur wrote:
> >                 if (card->tx_skb[port] == skb) {
> >                         skb_get(skb);
> >                         solos_pop(SKB_CB(skb)->vcc, skb);
> >                         SKB_CB(skb)->vcc = NULL;
> 
> Um... yes, that would probably work. But it's subtle enough that it
> bothers me. And if it *did* cause any strange issues, it's a rare case
> and would be hard to reproduce/debug. Is it *really* necessary, just to
> speed up the vcc close? I'm inclined to stick with the 'KISS' approach.

That's why I proposed that simple loop that just calls ->pop() and
sets vcc to NULL.

Forget about that, Chas said that we cannot leave close() until we
close that vcc, so we really need to wait.

Krzysiek
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c
index 9851093..aabe021 100644
--- a/drivers/atm/solos-pci.c
+++ b/drivers/atm/solos-pci.c
@@ -868,6 +868,19 @@  static void pclose(struct atm_vcc *vcc)
 	struct solos_card *card = vcc->dev->dev_data;
 	struct sk_buff *skb;
 	struct pkt_hdr *header;
+	unsigned int port;
+
+	tasklet_disable(&card->tlet);
+	spin_lock(&card->tx_queue_lock);
+	for (port = 0; port < card->nr_ports; port++)
+		skb_queue_walk(&card->tx_queue[port], skb)
+			if (SKB_CB(skb)->vcc == vcc) {
+				skb_get(skb);
+				solos_pop(SKB_CB(skb)->vcc, skb);
+				SKB_CB(skb)->vcc = NULL;
+			}
+	spin_unlock(&card->tx_queue_lock);
+	tasklet_enable(&card->tlet);
 
 	skb = alloc_skb(sizeof(*header), GFP_ATOMIC);
 	if (!skb) {