LinuxLists.cc - [git patches] net driver updates

[permalink] [raw]

Subject: Re: [git patches] net driver updates

Jeff Garzik wrote:
> Mostly fixes, a few cleanups (generally assisting fixes), and an
> exception for PS3 wireless because it had been posted, reviewed and
> acked for a while, just not committed.
>
> Please pull from 'upstream-davem' branch of
> master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/netdev-2.6.git upstream-davem
>
> to receive the following updates:
>
>
> drivers/net/cxgb3/sge.c | 35 +-
>
>
> diff --git a/drivers/net/cxgb3/sge.c b/drivers/net/cxgb3/sge.c
> index 9ca8c66..979f3fc 100644
> --- a/drivers/net/cxgb3/sge.c
> +++ b/drivers/net/cxgb3/sge.c
> @@ -1059,6 +1059,14 @@ static void write_tx_pkt_wr(struct adapter *adap, struct sk_buff *skb,
> htonl(V_WR_TID(q->token)));
> }
>
> +static inline void t3_stop_queue(struct net_device *dev, struct sge_qset *qs,
> + struct sge_txq *q)
> +{
> + netif_stop_queue(dev);
> + set_bit(TXQ_ETH, &qs->txq_stopped);
> + q->stops++;
> +}
> +
> /**
> * eth_xmit - add a packet to the Ethernet Tx queue
> * @skb: the packet
> @@ -1090,31 +1098,18 @@ int t3_eth_xmit(struct sk_buff *skb, struct net_device *dev)
> ndesc = calc_tx_descs(skb);
>
> if (unlikely(credits < ndesc)) {
> - if (!netif_queue_stopped(dev)) {
> - netif_stop_queue(dev);
> - set_bit(TXQ_ETH, &qs->txq_stopped);
> - q->stops++;
> - dev_err(&adap->pdev->dev,
> - "%s: Tx ring %u full while queue awake!\n",
> - dev->name, q->cntxt_id & 7);
> - }
> + t3_stop_queue(dev, qs, q);
> + dev_err(&adap->pdev->dev,
> + "%s: Tx ring %u full while queue awake!\n",
> + dev->name, q->cntxt_id & 7);
> spin_unlock(&q->lock);
> return NETDEV_TX_BUSY;
> }
>
> q->in_use += ndesc;
> - if (unlikely(credits - ndesc < q->stop_thres)) {
> - q->stops++;
> - netif_stop_queue(dev);
> - set_bit(TXQ_ETH, &qs->txq_stopped);
> -#if !USE_GTS
> - if (should_restart_tx(q) &&
> - test_and_clear_bit(TXQ_ETH, &qs->txq_stopped)) {
> - q->restarts++;
> - netif_wake_queue(dev);
> - }
> -#endif
> - }
> + if (unlikely(credits - ndesc < q->stop_thres))
> + if (USE_GTS || !should_restart_tx(q))
> + t3_stop_queue(dev, qs, q);
>
> gen = q->gen;
> q->unacked += ndesc;
>
Hi Jeff,

I thought I had NAK'ed the patch modifying sge.c from Krishna Kumar.
Looking back at my answer at the time, I was obviously unclear.
Can you please revert the drivers/net/cxgb3sge.c change ?

Cheers,
Divy

2008-02-12 05:56:52

[permalink] [raw]

Subject: Re: [git patches] net driver updates

From: Jeff Garzik <[email protected]>
Date: Mon, 11 Feb 2008 12:05:16 -0500

> Mostly fixes, a few cleanups (generally assisting fixes), and an
> exception for PS3 wireless because it had been posted, reviewed and
> acked for a while, just not committed.
>
> Please pull from 'upstream-davem' branch of
> master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/netdev-2.6.git upstream-davem
>
> to receive the following updates:

Pulled, thanks a lot Jeff!

2008-02-20 20:38:54

[permalink] [raw]

Subject: Re: [git patches] net driver updates

Divy Le Ray wrote:
> Jeff Garzik wrote:
>> Mostly fixes, a few cleanups (generally assisting fixes), and an
>> exception for PS3 wireless because it had been posted, reviewed and
>> acked for a while, just not committed.
>>
>> Please pull from 'upstream-davem' branch of
>> master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/netdev-2.6.git
>> upstream-davem
>>
>> to receive the following updates:
>>
>>
>> drivers/net/cxgb3/sge.c | 35 +-
>>
>> diff --git a/drivers/net/cxgb3/sge.c b/drivers/net/cxgb3/sge.c
>> index 9ca8c66..979f3fc 100644
>> --- a/drivers/net/cxgb3/sge.c
>> +++ b/drivers/net/cxgb3/sge.c
>> @@ -1059,6 +1059,14 @@ static void write_tx_pkt_wr(struct adapter
>> *adap, struct sk_buff *skb,
>> htonl(V_WR_TID(q->token)));
>> }
>>
>> +static inline void t3_stop_queue(struct net_device *dev, struct
>> sge_qset *qs,
>> + struct sge_txq *q)
>> +{
>> + netif_stop_queue(dev);
>> + set_bit(TXQ_ETH, &qs->txq_stopped);
>> + q->stops++;
>> +}
>> +
>> /**
>> * eth_xmit - add a packet to the Ethernet Tx queue
>> * @skb: the packet
>> @@ -1090,31 +1098,18 @@ int t3_eth_xmit(struct sk_buff *skb, struct
>> net_device *dev)
>> ndesc = calc_tx_descs(skb);
>>
>> if (unlikely(credits < ndesc)) {
>> - if (!netif_queue_stopped(dev)) {
>> - netif_stop_queue(dev);
>> - set_bit(TXQ_ETH, &qs->txq_stopped);
>> - q->stops++;
>> - dev_err(&adap->pdev->dev,
>> - "%s: Tx ring %u full while queue awake!\n",
>> - dev->name, q->cntxt_id & 7);
>> - }
>> + t3_stop_queue(dev, qs, q);
>> + dev_err(&adap->pdev->dev,
>> + "%s: Tx ring %u full while queue awake!\n",
>> + dev->name, q->cntxt_id & 7);
>> spin_unlock(&q->lock);
>> return NETDEV_TX_BUSY;
>> }
>>
>> q->in_use += ndesc;
>> - if (unlikely(credits - ndesc < q->stop_thres)) {
>> - q->stops++;
>> - netif_stop_queue(dev);
>> - set_bit(TXQ_ETH, &qs->txq_stopped);
>> -#if !USE_GTS
>> - if (should_restart_tx(q) &&
>> - test_and_clear_bit(TXQ_ETH, &qs->txq_stopped)) {
>> - q->restarts++;
>> - netif_wake_queue(dev);
>> - }
>> -#endif
>> - }
>> + if (unlikely(credits - ndesc < q->stop_thres))
>> + if (USE_GTS || !should_restart_tx(q))
>> + t3_stop_queue(dev, qs, q);
>>
>> gen = q->gen;
>> q->unacked += ndesc;
>>
> Hi Jeff,
>
> I thought I had NAK'ed the patch modifying sge.c from Krishna Kumar.
> Looking back at my answer at the time, I was obviously unclear.
> Can you please revert the drivers/net/cxgb3sge.c change ?

Explain why, so I can include it in the changelog please...

Jeff

2008-02-21 01:16:40

[permalink] [raw]

Subject: Re: [git patches] net driver updates

Jeff Garzik wrote:
> Divy Le Ray wrote:
>> Jeff Garzik wrote:
>>> Mostly fixes, a few cleanups (generally assisting fixes), and an
>>> exception for PS3 wireless because it had been posted, reviewed and
>>> acked for a while, just not committed.
>>>
>>> Please pull from 'upstream-davem' branch of
>>> master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/netdev-2.6.git
>>> upstream-davem
>>>
>>> to receive the following updates:
>>>
>>>
>>> drivers/net/cxgb3/sge.c | 35 +-
>>> diff --git a/drivers/net/cxgb3/sge.c b/drivers/net/cxgb3/sge.c
>>> index 9ca8c66..979f3fc 100644
>>> --- a/drivers/net/cxgb3/sge.c
>>> +++ b/drivers/net/cxgb3/sge.c
>>> @@ -1059,6 +1059,14 @@ static void write_tx_pkt_wr(struct adapter
>>> *adap, struct sk_buff *skb,
>>> htonl(V_WR_TID(q->token)));
>>> }
>>>
>>> +static inline void t3_stop_queue(struct net_device *dev, struct
>>> sge_qset *qs,
>>> + struct sge_txq *q)
>>> +{
>>> + netif_stop_queue(dev);
>>> + set_bit(TXQ_ETH, &qs->txq_stopped);
>>> + q->stops++;
>>> +}
>>> +
>>> /**
>>> * eth_xmit - add a packet to the Ethernet Tx queue
>>> * @skb: the packet
>>> @@ -1090,31 +1098,18 @@ int t3_eth_xmit(struct sk_buff *skb, struct
>>> net_device *dev)
>>> ndesc = calc_tx_descs(skb);
>>>
>>> if (unlikely(credits < ndesc)) {
>>> - if (!netif_queue_stopped(dev)) {
>>> - netif_stop_queue(dev);
>>> - set_bit(TXQ_ETH, &qs->txq_stopped);
>>> - q->stops++;
>>> - dev_err(&adap->pdev->dev,
>>> - "%s: Tx ring %u full while queue awake!\n",
>>> - dev->name, q->cntxt_id & 7);
>>> - }
>>> + t3_stop_queue(dev, qs, q);
>>> + dev_err(&adap->pdev->dev,
>>> + "%s: Tx ring %u full while queue awake!\n",
>>> + dev->name, q->cntxt_id & 7);
>>> spin_unlock(&q->lock);
>>> return NETDEV_TX_BUSY;
>>> }
>>>
>>> q->in_use += ndesc;
>>> - if (unlikely(credits - ndesc < q->stop_thres)) {
>>> - q->stops++;
>>> - netif_stop_queue(dev);
>>> - set_bit(TXQ_ETH, &qs->txq_stopped);
>>> -#if !USE_GTS
>>> - if (should_restart_tx(q) &&
>>> - test_and_clear_bit(TXQ_ETH, &qs->txq_stopped)) {
>>> - q->restarts++;
>>> - netif_wake_queue(dev);
>>> - }
>>> -#endif
>>> - }
>>> + if (unlikely(credits - ndesc < q->stop_thres))
>>> + if (USE_GTS || !should_restart_tx(q))
>>> + t3_stop_queue(dev, qs, q);
>>>
>>> gen = q->gen;
>>> q->unacked += ndesc;
>>>
>> Hi Jeff,
>>
>> I thought I had NAK'ed the patch modifying sge.c from Krishna Kumar.
>> Looking back at my answer at the time, I was obviously unclear.
>> Can you please revert the drivers/net/cxgb3sge.c change ?
>
> Explain why, so I can include it in the changelog please...

Hi Jeff,

The first part of the patch removes the !netif_queue_stopped(dev).
It opens the race discussed a while ago between Stephen hemminger and
David Miller:
http://marc.info/?l=linux-netdev&m=113383224512427&w=2

The second part of the patch breaks the current usage of txq_stopped and
the logic that stops and restarts the Tx queue.

Cheers,
Divy

2008-02-21 03:46:01

by Krishna Kumar2

[permalink] [raw]

Subject: Re: [git patches] net driver updates

Hi Divy,

> > Explain why, so I can include it in the changelog please...
>
> Hi Jeff,
>
> The first part of the patch removes the !netif_queue_stopped(dev).
> It opens the race discussed a while ago between Stephen hemminger and
> David Miller:
> http://marc.info/?l=linux-netdev&m=113383224512427&w=2

I feel this race cannot happen anymore. I think the fix for that race was
to introduce the
__LINK_STATE_QDISC_RUNNING bit thus eliminating any races between CPU's. If
one
CPU has called xmit, the other CPU will enqueue skbs (by holding the
queue_lock) and
exit from qdisc_run since it finds the bit set already.

Thanks,

- KK

2008-02-21 05:46:25

[permalink] [raw]

Subject: Re: [git patches] net driver updates

From: Krishna Kumar2 <[email protected]>
Date: Thu, 21 Feb 2008 09:13:49 +0530

> Hi Divy,
>
> > > Explain why, so I can include it in the changelog please...
> >
> > Hi Jeff,
> >
> > The first part of the patch removes the !netif_queue_stopped(dev).
> > It opens the race discussed a while ago between Stephen hemminger and
> > David Miller:
> > http://marc.info/?l=linux-netdev&m=113383224512427&w=2
>
> I feel this race cannot happen anymore. I think the fix for that race was
> to introduce the
> __LINK_STATE_QDISC_RUNNING bit thus eliminating any races between CPU's. If
> one
> CPU has called xmit, the other CPU will enqueue skbs (by holding the
> queue_lock) and
> exit from qdisc_run since it finds the bit set already.

And the race is talking about LLTX, which S2IO doesn't use as
far as I can tell.

2008-02-21 05:57:37

[permalink] [raw]

Subject: RE: [git patches] net driver updates

> -----Original Message-----
> From: David Miller [mailto:[email protected]]
> Sent: Wednesday, February 20, 2008 9:47 PM
> To: [email protected]
> Cc: Divy Le Ray; [email protected]; [email protected];
> [email protected]
> Subject: Re: [git patches] net driver updates
>
> From: Krishna Kumar2 <[email protected]>
> Date: Thu, 21 Feb 2008 09:13:49 +0530
>
> > Hi Divy,
> >
> > > > Explain why, so I can include it in the changelog please...
> > >
> > > Hi Jeff,
> > >
> > > The first part of the patch removes the !netif_queue_stopped(dev).
> > > It opens the race discussed a while ago between Stephen hemminger
> and
> > > David Miller:
> > > http://marc.info/?l=linux-netdev&m=113383224512427&w=2
> >
> > I feel this race cannot happen anymore. I think the fix for that
race
> was
> > to introduce the
> > __LINK_STATE_QDISC_RUNNING bit thus eliminating any races between
> CPU's. If
> > one
> > CPU has called xmit, the other CPU will enqueue skbs (by holding the
> > queue_lock) and
> > exit from qdisc_run since it finds the bit set already.
>
> And the race is talking about LLTX, which S2IO doesn't use as
> far as I can tell.

Dave,

The driver is cxgb3 here, it uses LLTX.

Cheers,
Divy

2008-02-21 06:03:39

by Krishna Kumar2

[permalink] [raw]

Subject: RE: [git patches] net driver updates

Hi Divy,

But the race doesn't exist even for LLTX drivers these days. There is no
way two cpu's can execute the xmit handler at the same time.

Thanks,

- KK

> > > > The first part of the patch removes the !netif_queue_stopped(dev).
> > > > It opens the race discussed a while ago between Stephen hemminger
> > and
> > > > David Miller:
> > > > http://marc.info/?l=linux-netdev&m=113383224512427&w=2
> > >
> > > I feel this race cannot happen anymore. I think the fix for that
> race
> > was
> > > to introduce the
> > > __LINK_STATE_QDISC_RUNNING bit thus eliminating any races between
> > CPU's. If
> > > one
> > > CPU has called xmit, the other CPU will enqueue skbs (by holding the
> > > queue_lock) and
> > > exit from qdisc_run since it finds the bit set already.
> >
> > And the race is talking about LLTX, which S2IO doesn't use as
> > far as I can tell.
>
> Dave,
>
> The driver is cxgb3 here, it uses LLTX.
>
> Cheers,
> Divy

2008-02-21 06:15:31

[permalink] [raw]

Subject: Re: [git patches] net driver updates

From: "Divy Le Ray" <[email protected]>
Date: Wed, 20 Feb 2008 21:57:08 -0800

> The driver is cxgb3 here, it uses LLTX.

That's extremely unfortunate, hopefully you can update it to
use a model like tg3 and others use. LLTX is a lost cause
for hardware device drivers, and in fact we'd like to remove
it tree wide eventually.

2008-02-21 06:54:42

[permalink] [raw]

Subject: RE: [git patches] net driver updates

> From: "Divy Le Ray" <[email protected]>
> Date: Wed, 20 Feb 2008 21:57:08 -0800
>
> > The driver is cxgb3 here, it uses LLTX.
>
> That's extremely unfortunate, hopefully you can update it to
> use a model like tg3 and others use. LLTX is a lost cause
> for hardware device drivers, and in fact we'd like to remove
> it tree wide eventually.

It sounds like I messed up.
cxgb3 does not use LLTX - The Chelsio driver does, thus should be
converted.

Divy

2008-02-25 22:59:30

[permalink] [raw]

Subject: Re: [git patches] net driver updates

Divy Le Ray wrote:
>> From: "Divy Le Ray" <[email protected]>
>> Date: Wed, 20 Feb 2008 21:57:08 -0800
>>
>>> The driver is cxgb3 here, it uses LLTX.
>> That's extremely unfortunate, hopefully you can update it to
>> use a model like tg3 and others use. LLTX is a lost cause
>> for hardware device drivers, and in fact we'd like to remove
>> it tree wide eventually.
>
> It sounds like I messed up.
> cxgb3 does not use LLTX - The Chelsio driver does, thus should be
> converted.

So, it sounds like Krishna's patch is ok, because the race does not
exist anymore in this driver?

Jeff

2008-02-25 23:32:30

by Marin Mitov

[permalink] [raw]

Subject: Re: [git patches] net driver updates

On Tuesday 26 February 2008 12:59:04 am you wrote:
> Divy Le Ray wrote:
> >> From: "Divy Le Ray" <[email protected]>
> >> Date: Wed, 20 Feb 2008 21:57:08 -0800
> >>
> >>> The driver is cxgb3 here, it uses LLTX.
> >>
> >> That's extremely unfortunate, hopefully you can update it to
> >> use a model like tg3 and others use. LLTX is a lost cause
> >> for hardware device drivers, and in fact we'd like to remove
> >> it tree wide eventually.

Just for info: loopback.c uses it too :-)

> >
> > It sounds like I messed up.
> > cxgb3 does not use LLTX - The Chelsio driver does, thus should be
> > converted.
>
> So, it sounds like Krishna's patch is ok, because the race does not
> exist anymore in this driver?
>
> Jeff
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

2008-02-25 23:36:39

[permalink] [raw]

Subject: Re: [git patches] net driver updates

Marin Mitov wrote:
> On Tuesday 26 February 2008 12:59:04 am you wrote:
>> Divy Le Ray wrote:
>>>> From: "Divy Le Ray" <[email protected]>
>>>> Date: Wed, 20 Feb 2008 21:57:08 -0800
>>>>
>>>>> The driver is cxgb3 here, it uses LLTX.
>>>> That's extremely unfortunate, hopefully you can update it to
>>>> use a model like tg3 and others use. LLTX is a lost cause
>>>> for hardware device drivers, and in fact we'd like to remove
>>>> it tree wide eventually.
>
> Just for info: loopback.c uses it too :-)

Yes, we're all aware of this. Google for "david miller loopback lltx"
for a few examples...

Also, please do not strip CC's, that's quite annoying.

Jeff

2008-02-26 00:13:41

[permalink] [raw]

Subject: Re: [git patches] net driver updates

>
> So, it sounds like Krishna's patch is ok, because the race does not
> exist anymore in this driver?
The first part is right indeed, but the second part is breaking the
current usage of txq_stopped and
the logic that stops and restarts the Tx queue.
I can submit a patch fixing it. Plese let me know what's more convenient
for you.

Cheers,
Divy

2008-02-26 00:18:00

[permalink] [raw]

Subject: Re: [git patches] net driver updates

Divy Le Ray wrote:
>
>>
>> So, it sounds like Krishna's patch is ok, because the race does not
>> exist anymore in this driver?
> The first part is right indeed, but the second part is breaking the
> current usage of txq_stopped and
> the logic that stops and restarts the Tx queue.
> I can submit a patch fixing it. Plese let me know what's more convenient
> for you.

At this point, yes a fix (with a good patch description) would be
preferred...

Thanks,

Jeff

2008-02-26 20:51:30