[PATCH net-next] r8169: Support for byte queue limits

[PATCH net-next] r8169: Support for byte queue limits

[igorm]

From: Igor Maravic <igorm@etf.rs>

Changes to r8169 to use byte queue limits.

Signed-off-by: Igor Maravic <igorm@etf.rs>
---
 drivers/net/ethernet/realtek/r8169.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 36b2a4b..1a6a12a 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5326,6 +5326,7 @@ static void rtl8169_tx_clear(struct rtl8169_private *tp)
 {
 	rtl8169_tx_clear_range(tp, tp->dirty_tx, NUM_TX_DESC);
 	tp->cur_tx = tp->dirty_tx = 0;
+	netdev_reset_queue(tp->dev);
 }
 
 static void rtl8169_schedule_work(struct net_device *dev, work_func_t task)
@@ -5546,6 +5547,8 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 
 	tp->cur_tx += frags + 1;
 
+	netdev_sent_queue(dev, skb->len);
+
 	wmb();
 
 	RTL_W8(TxPoll, NPQ);
@@ -5624,6 +5627,8 @@ static void rtl8169_tx_interrupt(struct net_device *dev,
 				 void __iomem *ioaddr)
 {
 	unsigned int dirty_tx, tx_left;
+	unsigned int bytes_compl = 0;
+	int tx_compl = 0;
 
 	dirty_tx = tp->dirty_tx;
 	smp_rmb();
@@ -5642,14 +5647,18 @@ static void rtl8169_tx_interrupt(struct net_device *dev,
 		rtl8169_unmap_tx_skb(&tp->pci_dev->dev, tx_skb,
 				     tp->TxDescArray + entry);
 		if (status & LastFrag) {
-			dev->stats.tx_packets++;
-			dev->stats.tx_bytes += tx_skb->skb->len;
+			tx_compl++;
+			bytes_compl += tx_skb->skb->len;
 			dev_kfree_skb(tx_skb->skb);
 			tx_skb->skb = NULL;
 		}
 		dirty_tx++;
 		tx_left--;
 	}
+	dev->stats.tx_packets += tx_compl;
+	dev->stats.tx_bytes += bytes_compl;
+
+	netdev_completed_queue(dev, tx_compl, bytes_compl);
 
 	if (tp->dirty_tx != dirty_tx) {
 		tp->dirty_tx = dirty_tx;
-- 
1.7.5.4

Re: [PATCH net-next] r8169: Support for byte queue limits

[Eric Dumazet]

Le lundi 19 décembre 2011 à 11:09 +0100, igorm@etf.rs a écrit :
> From: Igor Maravic <igorm@etf.rs>
> 
> Changes to r8169 to use byte queue limits.
> 
> Signed-off-by: Igor Maravic <igorm@etf.rs>
> ---
>  drivers/net/ethernet/realtek/r8169.c |   13 +++++++++++--
>  1 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 36b2a4b..1a6a12a 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -5326,6 +5326,7 @@ static void rtl8169_tx_clear(struct rtl8169_private *tp)
>  {
>  	rtl8169_tx_clear_range(tp, tp->dirty_tx, NUM_TX_DESC);
>  	tp->cur_tx = tp->dirty_tx = 0;
> +	netdev_reset_queue(tp->dev);
>  }
>  
>  static void rtl8169_schedule_work(struct net_device *dev, work_func_t task)
> @@ -5546,6 +5547,8 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
>  
>  	tp->cur_tx += frags + 1;
>  
> +	netdev_sent_queue(dev, skb->len);
> +

Hmm, thats a bit too late here. An interrupt could theorically free skb
right before your deref.


>  	wmb();
>  
>  	RTL_W8(TxPoll, NPQ);
> @@ -5624,6 +5627,8 @@ static void rtl8169_tx_interrupt(struct net_device *dev,

I believe its better to access skb _before_ the

txd->opts2 = cpu_to_le32(opts[1]);
wmb();

Re: [PATCH net-next] r8169: Support for byte queue limits

[David Miller]

From: igorm@etf.rs
Date: Mon, 19 Dec 2011 11:28:35 +0100

> From: Igor Maravic <igorm@etf.rs>
> 
> Changes to r8169 to use byte queue limits.
> 
> Signed-off-by: Igor Maravic <igorm@etf.rs>

I refuse to consider your work if you keep making such poor submissions
and ignoring the contribution that reviewers are making to your patches.

First of all, you must mention what changes you are making from one
version of your patch to another, so people can tell what is changing.

You also must credit suggestions made by others.  Otherwise peopl,
including me, will start to simply ignore you and all of your work.

Eric even told you these things explicitly, and you flat out ignored
him by submitting this patch again with no indication of what changed
from the last version and no credit to Eric for his tireless feedback
and suggestions.

This cannot continue, and you are on my black list for the rest of
the week and therefore I will ignore all of your patches during this
time.

You've been given many opportunities to correct your ways, yet you
still make extremely poor patch submissions, refuse to credit the
help others give to you, and generally waste our time.

[PATCH net-next] r8169: Support for byte queue limits

[igorm]

From: Igor Maravic <igorm@etf.rs>

Changes to r8169 to use byte queue limits.

Signed-off-by: Igor Maravic <igorm@etf.rs>
---
 drivers/net/ethernet/realtek/r8169.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 36b2a4b..5fefcc3 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5326,6 +5326,7 @@ static void rtl8169_tx_clear(struct rtl8169_private *tp)
 {
 	rtl8169_tx_clear_range(tp, tp->dirty_tx, NUM_TX_DESC);
 	tp->cur_tx = tp->dirty_tx = 0;
+	netdev_reset_queue(tp->dev);
 }
 
 static void rtl8169_schedule_work(struct net_device *dev, work_func_t task)
@@ -5535,6 +5536,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 		opts[0] |= FirstFrag | LastFrag;
 		tp->tx_skb[entry].skb = skb;
 	}
+	netdev_sent_queue(dev, skb->len);
 
 	txd->opts2 = cpu_to_le32(opts[1]);
 
@@ -5624,6 +5626,8 @@ static void rtl8169_tx_interrupt(struct net_device *dev,
 				 void __iomem *ioaddr)
 {
 	unsigned int dirty_tx, tx_left;
+	unsigned int bytes_compl = 0;
+	int tx_compl = 0;
 
 	dirty_tx = tp->dirty_tx;
 	smp_rmb();
@@ -5642,14 +5646,18 @@ static void rtl8169_tx_interrupt(struct net_device *dev,
 		rtl8169_unmap_tx_skb(&tp->pci_dev->dev, tx_skb,
 				     tp->TxDescArray + entry);
 		if (status & LastFrag) {
-			dev->stats.tx_packets++;
-			dev->stats.tx_bytes += tx_skb->skb->len;
+			tx_compl++;
+			bytes_compl += tx_skb->skb->len;
 			dev_kfree_skb(tx_skb->skb);
 			tx_skb->skb = NULL;
 		}
 		dirty_tx++;
 		tx_left--;
 	}
+	dev->stats.tx_packets += tx_compl;
+	dev->stats.tx_bytes += bytes_compl;
+
+	netdev_completed_queue(dev, tx_compl, bytes_compl);
 
 	if (tp->dirty_tx != dirty_tx) {
 		tp->dirty_tx = dirty_tx;
-- 
1.7.5.4

Re: [PATCH net-next] r8169: Support for byte queue limits

[Igor Maravić]

>
> What are the changes in this version, versus prior submissions ?
>

You didn't answer to my comments, so I thought that my version was OK.
I'l change this in a sec.

Re: [PATCH net-next] r8169: Support for byte queue limits

[Eric Dumazet]

Le lundi 19 décembre 2011 à 10:27 +0100, igorm@etf.rs a écrit :
> From: Igor Maravic <igorm@etf.rs>
> 
> Changes to r8169 to use byte queue limits.
> 
> Signed-off-by: Igor Maravic <igorm@etf.rs>
> ---
>  drivers/net/ethernet/realtek/r8169.c |   13 +++++++++++--
>  1 files changed, 11 insertions(+), 2 deletions(-)
> 

What are the changes in this version, versus prior submissions ?

>  
>  static void rtl8169_schedule_work(struct net_device *dev, work_func_t task)
> @@ -5460,6 +5461,8 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
>  		txd->opts1 |= cpu_to_le32(LastFrag);
>  	}
>  
> +	netdev_sent_queue(tp->dev, skb->len);
> +
>  	return cur_frag;
>  

Ignoring a review/comment/suggestion is not very wise, since I wont Ack
this patch as is, and I'll remember for your next patches that you dont
care of me taking the time to review your work.

Your choice.

Please call this from rtl8169_start_xmit() instead.
 
	netdev_sent_queue(dev, skb->len);

Re: [PATCH net-next] r8169: Support for byte queue limits

[Igor Maravić]

Resubmitted the patch. Sorry for double signoffs.
BR
Igor

[PATCH net-next] r8169: Support for byte queue limits

[igorm]

From: Igor Maravic <igorm@etf.rs>

Changes to r8169 to use byte queue limits.

Signed-off-by: Igor Maravic <igorm@etf.rs>
---
 drivers/net/ethernet/realtek/r8169.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 36b2a4b..bfdb5fd 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5326,6 +5326,7 @@ static void rtl8169_tx_clear(struct rtl8169_private *tp)
 {
 	rtl8169_tx_clear_range(tp, tp->dirty_tx, NUM_TX_DESC);
 	tp->cur_tx = tp->dirty_tx = 0;
+	netdev_reset_queue(tp->dev);
 }
 
 static void rtl8169_schedule_work(struct net_device *dev, work_func_t task)
@@ -5460,6 +5461,8 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
 		txd->opts1 |= cpu_to_le32(LastFrag);
 	}
 
+	netdev_sent_queue(tp->dev, skb->len);
+
 	return cur_frag;
 
 err_out:
@@ -5624,6 +5627,8 @@ static void rtl8169_tx_interrupt(struct net_device *dev,
 				 void __iomem *ioaddr)
 {
 	unsigned int dirty_tx, tx_left;
+	unsigned int bytes_compl = 0;
+	int tx_compl = 0;
 
 	dirty_tx = tp->dirty_tx;
 	smp_rmb();
@@ -5642,14 +5647,18 @@ static void rtl8169_tx_interrupt(struct net_device *dev,
 		rtl8169_unmap_tx_skb(&tp->pci_dev->dev, tx_skb,
 				     tp->TxDescArray + entry);
 		if (status & LastFrag) {
-			dev->stats.tx_packets++;
-			dev->stats.tx_bytes += tx_skb->skb->len;
+			tx_compl++;
+			bytes_compl += tx_skb->skb->len;
 			dev_kfree_skb(tx_skb->skb);
 			tx_skb->skb = NULL;
 		}
 		dirty_tx++;
 		tx_left--;
 	}
+	dev->stats.tx_packets += tx_compl;
+	dev->stats.tx_bytes += bytes_compl;
+
+	netdev_completed_queue(dev, tx_compl, bytes_compl);
 
 	if (tp->dirty_tx != dirty_tx) {
 		tp->dirty_tx = dirty_tx;
-- 
1.7.5.4

Re: [PATCH net-next] r8169: Support for byte queue limits

[Igor Maravić]

> Why here in rtl8169_xmit_frags() ?
>
> If done from rtl8169_start_xmit(), you already have "dev" pointer.
>
>        netdev_sent_queue(dev, skb->len);
>

I put it there because actual dma maping is occurring in that function.
Logically there is no difference if I put it after calling of function
rtl8169_xmit_frags() in rtl8169_start_xmit().

If You think it would be better to be in function rtl8169_start_xmit,
for performance reasons I'l put it there.

In any case I'l resubmit this patch in Monday. After all it's Saturday :)
BR
Igor

Re: [PATCH net-next] r8169: Support for byte queue limits

[Eric Dumazet]

Le lundi 12 décembre 2011 à 05:00 -0800, igorm@etf.rs a écrit :
> From: Igor Maravic <igorm@etf.rs>
> 
> Changes to r8169 to use byte queue limits.
> 
> Signed-off-by: Igor Maravic <igorm@etf.rs>
> Signed-off-by: Igor Maravić <igorm@etf.rs>

Dont forget to CC maintainer of this driver.

> ---
>  drivers/net/ethernet/realtek/r8169.c |   13 +++++++++++--
>  1 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 36b2a4b..bfdb5fd 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -5326,6 +5326,7 @@ static void rtl8169_tx_clear(struct rtl8169_private *tp)
>  {
>  	rtl8169_tx_clear_range(tp, tp->dirty_tx, NUM_TX_DESC);
>  	tp->cur_tx = tp->dirty_tx = 0;
> +	netdev_reset_queue(tp->dev);
>  }
>  
>  static void rtl8169_schedule_work(struct net_device *dev, work_func_t task)
> @@ -5460,6 +5461,8 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
>  		txd->opts1 |= cpu_to_le32(LastFrag);
>  	}
>  
> +	netdev_sent_queue(tp->dev, skb->len);
> +

Why here in rtl8169_xmit_frags() ?

If done from rtl8169_start_xmit(), you already have "dev" pointer.

	netdev_sent_queue(dev, skb->len);

Re: [PATCH net-next] r8169: Support for byte queue limits

[David Miller]

From: igorm@etf.rs
Date: Mon, 12 Dec 2011 05:00:34 -0800

> From: Igor Maravic <igorm@etf.rs>
> 
> Changes to r8169 to use byte queue limits.
> 
> Signed-off-by: Igor Maravic <igorm@etf.rs>
> Signed-off-by: Igor Maravić <igorm@etf.rs>

Duplicate signoffs?

Someone please review this patch, thanks.

[PATCH net-next] r8169: Support for byte queue limits

[igorm]

From: Igor Maravic <igorm@etf.rs>

Changes to r8169 to use byte queue limits.

Signed-off-by: Igor Maravic <igorm@etf.rs>
Signed-off-by: Igor Maravić <igorm@etf.rs>
---
 drivers/net/ethernet/realtek/r8169.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 36b2a4b..bfdb5fd 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5326,6 +5326,7 @@ static void rtl8169_tx_clear(struct rtl8169_private *tp)
 {
 	rtl8169_tx_clear_range(tp, tp->dirty_tx, NUM_TX_DESC);
 	tp->cur_tx = tp->dirty_tx = 0;
+	netdev_reset_queue(tp->dev);
 }
 
 static void rtl8169_schedule_work(struct net_device *dev, work_func_t task)
@@ -5460,6 +5461,8 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
 		txd->opts1 |= cpu_to_le32(LastFrag);
 	}
 
+	netdev_sent_queue(tp->dev, skb->len);
+
 	return cur_frag;
 
 err_out:
@@ -5624,6 +5627,8 @@ static void rtl8169_tx_interrupt(struct net_device *dev,
 				 void __iomem *ioaddr)
 {
 	unsigned int dirty_tx, tx_left;
+	unsigned int bytes_compl = 0;
+	int tx_compl = 0;
 
 	dirty_tx = tp->dirty_tx;
 	smp_rmb();
@@ -5642,14 +5647,18 @@ static void rtl8169_tx_interrupt(struct net_device *dev,
 		rtl8169_unmap_tx_skb(&tp->pci_dev->dev, tx_skb,
 				     tp->TxDescArray + entry);
 		if (status & LastFrag) {
-			dev->stats.tx_packets++;
-			dev->stats.tx_bytes += tx_skb->skb->len;
+			tx_compl++;
+			bytes_compl += tx_skb->skb->len;
 			dev_kfree_skb(tx_skb->skb);
 			tx_skb->skb = NULL;
 		}
 		dirty_tx++;
 		tx_left--;
 	}
+	dev->stats.tx_packets += tx_compl;
+	dev->stats.tx_bytes += bytes_compl;
+
+	netdev_completed_queue(dev, tx_compl, bytes_compl);
 
 	if (tp->dirty_tx != dirty_tx) {
 		tp->dirty_tx = dirty_tx;
-- 
1.7.5.4

Re: [PATCH net-next] r8169: Support for byte queue limits

[David Miller]

From: Igor Maravić <igorm@etf.rs>
Date: Tue, 6 Dec 2011 22:36:21 +0100

>> Well, you're targetting this change to the net-next tree, are Francois's
>> fixes there?
> 
> I thought when You say "Applied" in reply, you push patches to net-next tree.

They were bug fixes, so they went to the 'net' tree.  So you have to wait
until I merge the net tree into net-next.

Re: [PATCH net-next] r8169: Support for byte queue limits

[Igor Maravić]

> Well, you're targetting this change to the net-next tree, are Francois's
> fixes there?

I thought when You say "Applied" in reply, you push patches to net-next tree.
My mistake. Still learning :)
I'l wait.

Re: [PATCH net-next] r8169: Support for byte queue limits

[David Miller]

From: igorm@etf.rs
Date: Tue,  6 Dec 2011 10:13:44 +0100

> From: Igor Maravic <igorm@etf.rs>
> 
> Changes to r8169 to use byte queue limits.
> 
> Signed-off-by: Igor Maravic <igorm@etf.rs>

Your change is only valid after Francois's fixes right?  That's what you
say in another email.

Well, you're targetting this change to the net-next tree, are Francois's
fixes there?

They aren't, so you need to be patient and not be so eager like a rabbit
to keep mindlessly pumping this patch to the list.

Re: [PATCH net-next] r8169: Support for byte queue limits

[Igor Maravić]

I'm resubmiting this patch, because Francois have fixed Rx FIFO
overflowing and  Rx index race between FIFO overflow recovery and NAPI
handler.
BR
Igor

[PATCH net-next] r8169: Support for byte queue limits

[igorm]

From: Igor Maravic <igorm@etf.rs>

Changes to r8169 to use byte queue limits.

Signed-off-by: Igor Maravic <igorm@etf.rs>
---
 drivers/net/ethernet/realtek/r8169.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 36b2a4b..bfdb5fd 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5326,6 +5326,7 @@ static void rtl8169_tx_clear(struct rtl8169_private *tp)
 {
 	rtl8169_tx_clear_range(tp, tp->dirty_tx, NUM_TX_DESC);
 	tp->cur_tx = tp->dirty_tx = 0;
+	netdev_reset_queue(tp->dev);
 }
 
 static void rtl8169_schedule_work(struct net_device *dev, work_func_t task)
@@ -5460,6 +5461,8 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
 		txd->opts1 |= cpu_to_le32(LastFrag);
 	}
 
+	netdev_sent_queue(tp->dev, skb->len);
+
 	return cur_frag;
 
 err_out:
@@ -5624,6 +5627,8 @@ static void rtl8169_tx_interrupt(struct net_device *dev,
 				 void __iomem *ioaddr)
 {
 	unsigned int dirty_tx, tx_left;
+	unsigned int bytes_compl = 0;
+	int tx_compl = 0;
 
 	dirty_tx = tp->dirty_tx;
 	smp_rmb();
@@ -5642,14 +5647,18 @@ static void rtl8169_tx_interrupt(struct net_device *dev,
 		rtl8169_unmap_tx_skb(&tp->pci_dev->dev, tx_skb,
 				     tp->TxDescArray + entry);
 		if (status & LastFrag) {
-			dev->stats.tx_packets++;
-			dev->stats.tx_bytes += tx_skb->skb->len;
+			tx_compl++;
+			bytes_compl += tx_skb->skb->len;
 			dev_kfree_skb(tx_skb->skb);
 			tx_skb->skb = NULL;
 		}
 		dirty_tx++;
 		tx_left--;
 	}
+	dev->stats.tx_packets += tx_compl;
+	dev->stats.tx_bytes += bytes_compl;
+
+	netdev_completed_queue(dev, tx_compl, bytes_compl);
 
 	if (tp->dirty_tx != dirty_tx) {
 		tp->dirty_tx = dirty_tx;
-- 
1.7.5.4

Re: [PATCH net-next] r8169: Support for byte queue limits

[Eric Dumazet]

Le vendredi 02 décembre 2011 à 22:54 +0100, Igor Maravić a écrit :
> >
> > Reread what I said : "BQL must be lightweight"
> >
> > Not : "No lock should be used"
> >
> > OK ?
> >
> 
> I'm out of ideas.
> 
> Do you think, if I remove netdev_reset_queue(tp->dev); from
> rtl8169_init_ring_indexes,
> and spin_locks, of course, that would be a good solution.
> 
> As far as I could see in marvell/sky2.c,  sfc/tx.c and intel/e1000e/netdev.c
> netdev_completed is called with out any lock.
> Please correct me if I'm wrong.

These drivers have a clean and separate start_xmit() and xmit_completion
path, each one being correctly serialized. No extra lock needed.

In the case of r8169, we are still trying to get the driver in a clean
state (without races).

Then, we'll add BQL, and it will be as easy as other drivers.

Re: [PATCH net-next] r8169: Support for byte queue limits

[Francois Romieu]

Eric Dumazet <eric.dumazet@gmail.com> :
[...]
> Also please CC maintainers of this driver to your submission.
> 
> (I did this)

Thanks.

Frankly I would appreciate if people could wait a bit until fixes for
the current RxFIFOOverflow mess - and a bit more - are submitted. I just
need the week end for some sanity tested patches with a few different
nic.

Btw some code may be saved with an adequate ad-hoc {bytes/tx}_compl
struct and a few helpers acting on it as well as dev/dev->stats and
skb.

--
Ueimor

Re: [PATCH net-next] r8169: Support for byte queue limits

[Igor Maravić]

>
> Reread what I said : "BQL must be lightweight"
>
> Not : "No lock should be used"
>
> OK ?
>

I'm out of ideas.

Do you think, if I remove netdev_reset_queue(tp->dev); from
rtl8169_init_ring_indexes,
and spin_locks, of course, that would be a good solution.

As far as I could see in marvell/sky2.c,  sfc/tx.c and intel/e1000e/netdev.c
netdev_completed is called with out any lock.
Please correct me if I'm wrong.
BR
Igor

Re: [PATCH net-next] r8169: Support for byte queue limits

[Eric Dumazet]

Le vendredi 02 décembre 2011 à 18:00 +0100, Igor Maravić a écrit :
> >
> > They are initted at device setup.
> >
> 
> What about when device is reseted?
> 

What happens to queued skbs ? Are they LOST ?

Think about it.


> >
> > Oh well, no, dont do that. BQL must be lightweight.
> >
> 
> Forcedeth driver utilizes spin_locks. :)

No added spin_locks in BQL patch.

Reread what I said : "BQL must be lightweight"

Not : "No lock should be used"

OK ?

Re: [PATCH net-next] r8169: Support for byte queue limits

[Igor Maravić]

>
> They are initted at device setup.
>

What about when device is reseted?

>
> Oh well, no, dont do that. BQL must be lightweight.
>

Forcedeth driver utilizes spin_locks. :)

Re: [PATCH net-next] r8169: Support for byte queue limits

[Eric Dumazet]

Le vendredi 02 décembre 2011 à 17:31 +0100, "Igor Maravić" a écrit :
> >
> > Quite frankly, any queued skb is eventually freed, so
> > netdev_reset_queue() could be avoided.
> >
> 
> If I didn't add netdev_reset_queue(), how dql would reset its parameters to 0?
> 

They are initted at device setup.

As I said, if every transmitted skb is correctly freed (and BQL
accounted), there is no need to netdev_reset_queue()

> I added spin_locks around netdev_completed_queue and netdev_sent_queue
> 

Oh well, no, dont do that. BQL must be lightweight.

Re: [PATCH net-next] r8169: Support for byte queue limits

["Igor Maravić"]

>
> Quite frankly, any queued skb is eventually freed, so
> netdev_reset_queue() could be avoided.
>

If I didn't add netdev_reset_queue(), how dql would reset its parameters to 0?

I added spin_locks around netdev_completed_queue and netdev_sent_queue

[PATCH net-next] r8169: Support for byte queue limits

Changes to r8169 to use byte queue limits.

Signed-off-by: Igor Maravic <igorm@etf.rs>
---
 drivers/net/ethernet/realtek/r8169.c |   17 +++++++++++++++--
 1 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index e5a6d8e..e46d4e6 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -3773,6 +3773,7 @@ static void rtl_init_rxcfg(struct rtl8169_private *tp)
 static void rtl8169_init_ring_indexes(struct rtl8169_private *tp)
 {
 	tp->dirty_tx = tp->dirty_rx = tp->cur_tx = tp->cur_rx = 0;
+	netdev_reset_queue(tp->dev);
 }

 static void rtl_hw_jumbo_enable(struct rtl8169_private *tp)
@@ -5422,6 +5423,7 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
 	unsigned int cur_frag, entry;
 	struct TxDesc * uninitialized_var(txd);
 	struct device *d = &tp->pci_dev->dev;
+	unsigned long flag;

 	entry = tp->cur_tx;
 	for (cur_frag = 0; cur_frag < info->nr_frags; cur_frag++) {
@@ -5458,6 +5460,9 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
 		tp->tx_skb[entry].skb = skb;
 		txd->opts1 |= cpu_to_le32(LastFrag);
 	}
+	spin_lock_irqsave(&tp->lock,flag);
+	netdev_sent_queue(tp->dev, skb->len);
+	spin_unlock_irqrestore(&tp->lock,flag);

 	return cur_frag;

@@ -5623,6 +5628,9 @@ static void rtl8169_tx_interrupt(struct net_device *dev,
 				 void __iomem *ioaddr)
 {
 	unsigned int dirty_tx, tx_left;
+	unsigned int bytes_compl = 0;
+	int tx_compl = 0;
+	unsigned long flag;

 	dirty_tx = tp->dirty_tx;
 	smp_rmb();
@@ -5641,8 +5649,8 @@ static void rtl8169_tx_interrupt(struct net_device *dev,
 		rtl8169_unmap_tx_skb(&tp->pci_dev->dev, tx_skb,
 				     tp->TxDescArray + entry);
 		if (status & LastFrag) {
-			dev->stats.tx_packets++;
-			dev->stats.tx_bytes += tx_skb->skb->len;
+			bytes_compl += tx_skb->skb->len;
+			tx_compl++;
 			dev_kfree_skb(tx_skb->skb);
 			tx_skb->skb = NULL;
 		}
@@ -5650,6 +5658,11 @@ static void rtl8169_tx_interrupt(struct net_device *dev,
 		tx_left--;
 	}

+	dev->stats.tx_packets += tx_compl;
+	dev->stats.tx_bytes += bytes_compl;
+	spin_lock_irqsave(&tp->lock, flag);
+	netdev_completed_queue(dev, tx_compl, bytes_compl);
+	spin_unlock_irqrestore(&tp->lock, flag);
 	if (tp->dirty_tx != dirty_tx) {
 		tp->dirty_tx = dirty_tx;
 		smp_wmb();
-- 
1.7.5.4

Re: [PATCH net-next] r8169: Support for byte queue limits

[Eric Dumazet]

Le vendredi 02 décembre 2011 à 15:54 +0100, Igor Maravić a écrit :
> > Hmm... What about rtl8169_tx_clear_range() or rtl8169_tx_clear() ?
> >
> 
> You mean we should put netdev_reset_queue() in those functions?
> 
> I think that we shouldn't put any thing in these functions.
> 
> When ever they are called, they are precede/followed by function
> rtl8169_hw_reset() which calls rtl_hw_reset() which calls
> rtl8169_init_ring_indexes() in which I've put netdev_reset_queue()
> 
> Only time when rtl8169_tx_clear_range() isn't preceded/followed by
> rtl8169_hw_reset() is in function rtl8169_xmit_frags(). In that
> function it's
> called only in case there's an error. In case there was an error in that
> function, netdev_sent_queue() will never be called, so we are going to have
> accurate length of queue.

Quite frankly, any queued skb is eventually freed, so
netdev_reset_queue() could be avoided.

r8169 driver has some problems (see current threads on netdev about it),
and its not very clear that BQL locking requirements are going to be
respected. This can lead to inaccurate BQL counts and a txqueue freeze.

(In particular, a chip reset can be triggered by an IRQ, while another
cpu is currently sending an skb through ndo_start_xmit())

So I advise not using netdev_reset_queue() in this driver, only
netdev_completed_queue(), and make sure that their calls are serialized
under a given lock (see include/linux/dynamic_queue_limits.h comments)

Re: [PATCH net-next] r8169: Support for byte queue limits

[Igor Maravić]

> Hmm... What about rtl8169_tx_clear_range() or rtl8169_tx_clear() ?
>

You mean we should put netdev_reset_queue() in those functions?

I think that we shouldn't put any thing in these functions.

When ever they are called, they are precede/followed by function
rtl8169_hw_reset() which calls rtl_hw_reset() which calls
rtl8169_init_ring_indexes() in which I've put netdev_reset_queue()

Only time when rtl8169_tx_clear_range() isn't preceded/followed by
rtl8169_hw_reset() is in function rtl8169_xmit_frags(). In that
function it's
called only in case there's an error. In case there was an error in that
function, netdev_sent_queue() will never be called, so we are going to have
accurate length of queue.

Re: [PATCH net-next] r8169: Support for byte queue limits

[Eric Dumazet]

Le vendredi 02 décembre 2011 à 14:09 +0100, igorm@etf.rs a écrit :
> From: Igor Maravic <igorm@etf.rs>
> 
> Changes to r8169 to use byte queue limits.
> 
> Signed-off-by: Igor Maravic <igorm@etf.rs>
> ---
>  drivers/net/ethernet/realtek/r8169.c |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index e5a6d8e..7ea428c 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -3773,6 +3773,7 @@ static void rtl_init_rxcfg(struct rtl8169_private *tp)
>  static void rtl8169_init_ring_indexes(struct rtl8169_private *tp)
>  {
>  	tp->dirty_tx = tp->dirty_rx = tp->cur_tx = tp->cur_rx = 0;
> +	netdev_reset_queue(tp->dev);


Hmm... What about rtl8169_tx_clear_range() or rtl8169_tx_clear() ?

Also please CC maintainers of this driver to your submission.

(I did this)

>  }
>  
>  static void rtl_hw_jumbo_enable(struct rtl8169_private *tp)
> @@ -5458,6 +5459,7 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
>  		tp->tx_skb[entry].skb = skb;
>  		txd->opts1 |= cpu_to_le32(LastFrag);
>  	}
> +	netdev_sent_queue(tp->dev, skb->len);
>  
>  	return cur_frag;
>  
> @@ -5623,6 +5625,8 @@ static void rtl8169_tx_interrupt(struct net_device *dev,
>  				 void __iomem *ioaddr)
>  {
>  	unsigned int dirty_tx, tx_left;
> +	unsigned int bytes_compl = 0;
> +	int tx_compl = 0;
>  
>  	dirty_tx = tp->dirty_tx;
>  	smp_rmb();
> @@ -5641,8 +5645,8 @@ static void rtl8169_tx_interrupt(struct net_device *dev,
>  		rtl8169_unmap_tx_skb(&tp->pci_dev->dev, tx_skb,
>  				     tp->TxDescArray + entry);
>  		if (status & LastFrag) {
> -			dev->stats.tx_packets++;
> -			dev->stats.tx_bytes += tx_skb->skb->len;
> +			bytes_compl += tx_skb->skb->len;
> +			tx_compl++;
>  			dev_kfree_skb(tx_skb->skb);
>  			tx_skb->skb = NULL;
>  		}
> @@ -5650,6 +5654,10 @@ static void rtl8169_tx_interrupt(struct net_device *dev,
>  		tx_left--;
>  	}
>  
> +	dev->stats.tx_packets += tx_compl;
> +	dev->stats.tx_bytes += bytes_compl;
> +	netdev_completed_queue(dev, tx_compl, bytes_compl);
> +
>  	if (tp->dirty_tx != dirty_tx) {
>  		tp->dirty_tx = dirty_tx;
>  		smp_wmb();

[PATCH net-next] r8169: Support for byte queue limits

[igorm]

From: Igor Maravic <igorm@etf.rs>

Changes to r8169 to use byte queue limits.

Signed-off-by: Igor Maravic <igorm@etf.rs>
---
 drivers/net/ethernet/realtek/r8169.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index e5a6d8e..7ea428c 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -3773,6 +3773,7 @@ static void rtl_init_rxcfg(struct rtl8169_private *tp)
 static void rtl8169_init_ring_indexes(struct rtl8169_private *tp)
 {
 	tp->dirty_tx = tp->dirty_rx = tp->cur_tx = tp->cur_rx = 0;
+	netdev_reset_queue(tp->dev);
 }
 
 static void rtl_hw_jumbo_enable(struct rtl8169_private *tp)
@@ -5458,6 +5459,7 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
 		tp->tx_skb[entry].skb = skb;
 		txd->opts1 |= cpu_to_le32(LastFrag);
 	}
+	netdev_sent_queue(tp->dev, skb->len);
 
 	return cur_frag;
 
@@ -5623,6 +5625,8 @@ static void rtl8169_tx_interrupt(struct net_device *dev,
 				 void __iomem *ioaddr)
 {
 	unsigned int dirty_tx, tx_left;
+	unsigned int bytes_compl = 0;
+	int tx_compl = 0;
 
 	dirty_tx = tp->dirty_tx;
 	smp_rmb();
@@ -5641,8 +5645,8 @@ static void rtl8169_tx_interrupt(struct net_device *dev,
 		rtl8169_unmap_tx_skb(&tp->pci_dev->dev, tx_skb,
 				     tp->TxDescArray + entry);
 		if (status & LastFrag) {
-			dev->stats.tx_packets++;
-			dev->stats.tx_bytes += tx_skb->skb->len;
+			bytes_compl += tx_skb->skb->len;
+			tx_compl++;
 			dev_kfree_skb(tx_skb->skb);
 			tx_skb->skb = NULL;
 		}
@@ -5650,6 +5654,10 @@ static void rtl8169_tx_interrupt(struct net_device *dev,
 		tx_left--;
 	}
 
+	dev->stats.tx_packets += tx_compl;
+	dev->stats.tx_bytes += bytes_compl;
+	netdev_completed_queue(dev, tx_compl, bytes_compl);
+
 	if (tp->dirty_tx != dirty_tx) {
 		tp->dirty_tx = dirty_tx;
 		smp_wmb();
-- 
1.7.5.4