The QDisc code does a bunch of locking which is unnecessary if
you have hardware which handles all of the queueing. Add
support for this, and skip over all of the queueing code if
the feature is enabled on a given device, which breaks QDisc
support on dpaa_eth, and also coopts the FCOE feature bit.
Signed-off-by: Andy Fleming <[email protected]>
Signed-off-by: Ben Collins <[email protected]>
Cc: [email protected]
---
include/linux/netdev_features.h | 2 ++
net/core/dev.c | 6 ++++++
2 files changed, 8 insertions(+)
diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 3dd3934..ffb4587 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -56,6 +56,7 @@ enum {
NETIF_F_LOOPBACK_BIT, /* Enable loopback */
NETIF_F_RXFCS_BIT, /* Append FCS to skb pkt data */
NETIF_F_RXALL_BIT, /* Receive errored frames too */
+ NETIF_F_HW_QDISC_BIT, /* Supports hardware Qdisc */
/*
* Add your fresh new feature above and remember to update
@@ -80,6 +81,7 @@ enum {
#define NETIF_F_GSO_ROBUST __NETIF_F(GSO_ROBUST)
#define NETIF_F_HIGHDMA __NETIF_F(HIGHDMA)
#define NETIF_F_HW_CSUM __NETIF_F(HW_CSUM)
+#define NETIF_F_HW_QDISC __NETIF_F(HW_QDISC)
#define NETIF_F_HW_VLAN_FILTER __NETIF_F(HW_VLAN_FILTER)
#define NETIF_F_HW_VLAN_RX __NETIF_F(HW_VLAN_RX)
#define NETIF_F_HW_VLAN_TX __NETIF_F(HW_VLAN_TX)
diff --git a/net/core/dev.c b/net/core/dev.c
index dffbef7..6818b18 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2743,6 +2743,12 @@ int dev_queue_xmit(struct sk_buff *skb)
skb_update_prio(skb);
+ if (dev->features & NETIF_F_HW_QDISC) {
+ txq = netdev_pick_tx(dev, skb);
+ rc = dev_hard_start_xmit(skb, dev, txq);
+ goto out;
+ }
+
txq = netdev_pick_tx(dev, skb);
q = rcu_dereference_bh(txq->qdisc);
--
1.8.1.2
From: Andy Fleming <[email protected]>
Date: Wed, 13 Jul 2011 08:52:04 -0500
> The QDisc code does a bunch of locking which is unnecessary if
> you have hardware which handles all of the queueing. Add
> support for this, and skip over all of the queueing code if
> the feature is enabled on a given device, which breaks QDisc
> support on dpaa_eth, and also coopts the FCOE feature bit.
>
> Signed-off-by: Andy Fleming <[email protected]>
> Signed-off-by: Ben Collins <[email protected]>
Sorry, no.
If we are going to support something like this then there needs to
be full coordination, configuration wise, so that if we enable
a qdisc that the hardware supports we submit it directly, but if
we enable a qdisc the HW does not support, we still use the software
qdisc.
This also means that we need to have a way to determine if the qdisc
configuration exceeds that parametorial limits of the device's HW
capabilities, and fallback to software qdisc in those cases too.
On Wed, 2011-07-13 at 08:52 -0500, Andy Fleming wrote:
> The QDisc code does a bunch of locking which is unnecessary if
> you have hardware which handles all of the queueing. Add
> support for this, and skip over all of the queueing code if
> the feature is enabled on a given device, which breaks QDisc
> support on dpaa_eth, and also coopts the FCOE feature bit.
>
> Signed-off-by: Andy Fleming <[email protected]>
> Signed-off-by: Ben Collins <[email protected]>
> Cc: [email protected]
> ---
> include/linux/netdev_features.h | 2 ++
> net/core/dev.c | 6 ++++++
> 2 files changed, 8 insertions(+)
>
> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> index 3dd3934..ffb4587 100644
> --- a/include/linux/netdev_features.h
> +++ b/include/linux/netdev_features.h
> @@ -56,6 +56,7 @@ enum {
> NETIF_F_LOOPBACK_BIT, /* Enable loopback */
> NETIF_F_RXFCS_BIT, /* Append FCS to skb pkt data */
> NETIF_F_RXALL_BIT, /* Receive errored frames too */
> + NETIF_F_HW_QDISC_BIT, /* Supports hardware Qdisc */
>
> /*
> * Add your fresh new feature above and remember to update
> @@ -80,6 +81,7 @@ enum {
> #define NETIF_F_GSO_ROBUST __NETIF_F(GSO_ROBUST)
> #define NETIF_F_HIGHDMA __NETIF_F(HIGHDMA)
> #define NETIF_F_HW_CSUM __NETIF_F(HW_CSUM)
> +#define NETIF_F_HW_QDISC __NETIF_F(HW_QDISC)
> #define NETIF_F_HW_VLAN_FILTER __NETIF_F(HW_VLAN_FILTER)
> #define NETIF_F_HW_VLAN_RX __NETIF_F(HW_VLAN_RX)
> #define NETIF_F_HW_VLAN_TX __NETIF_F(HW_VLAN_TX)
> diff --git a/net/core/dev.c b/net/core/dev.c
> index dffbef7..6818b18 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2743,6 +2743,12 @@ int dev_queue_xmit(struct sk_buff *skb)
>
> skb_update_prio(skb);
>
> + if (dev->features & NETIF_F_HW_QDISC) {
> + txq = netdev_pick_tx(dev, skb);
> + rc = dev_hard_start_xmit(skb, dev, txq);
> + goto out;
> + }
> +
> txq = netdev_pick_tx(dev, skb);
> q = rcu_dereference_bh(txq->qdisc);
>
Nobody forces you to use a qdisc.
And if your device really is lockless, it can use NETIF_F_LLTX feature.
From: Fleming Andy-AFLEMING <[email protected]>
Date: Fri, 22 Mar 2013 14:31:50 +0000
> It would appear one of our customers is attempting to upstream our
> code for us. We are aware that this current solution is unacceptable
> (which is why we have not submitted it), and we are currently trying
> to develop a less hacky solution that integrates with qdisc.
Ben, can can you coordinate with people instead of doing crap like
this?
On Mar 22, 2013, at 10:33 AM, David Miller <[email protected]> wrote:
> From: Fleming Andy-AFLEMING <[email protected]>
> Date: Fri, 22 Mar 2013 14:31:50 +0000
>
>> It would appear one of our customers is attempting to upstream our
>> code for us. We are aware that this current solution is unacceptable
>> (which is why we have not submitted it), and we are currently trying
>> to develop a less hacky solution that integrates with qdisc.
>
> Ben, can can you coordinate with people instead of doing crap like
> this?
"For us" is a loose term, when it's more that we are attempting to upstream code so our system is supported by a mainline kernel instead of having one-off kernels.
And we have been talking with Freescale about this for quite some time (couple years?). They have a roadmap that doesn't include getting this driver supported in mainline any time soon, so I'm taking time to get this done for our own system. I'm not meaning to step on any toes.
Believe me, I've attempted to make this as painless as possible. Only having 5 patches (and each just a few lines) is significantly less than how this driver started out. I can toss out the QDisc patch and work on a better way. I'm not totally familiar with the queueing, so any pointers to a better way to handle this would be appreciated.
--
Servergy : http://www.servergy.com/
SwissDisk : http://www.swissdisk.com/
Ubuntu : http://www.ubuntu.com/
My Blog : http://ben-collins.blogspot.com/
On Mar 22, 2013, at 9:11, "David Miller" <[email protected]> wrote:
> From: Andy Fleming <[email protected]>
> Date: Wed, 13 Jul 2011 08:52:04 -0500
>
>> The QDisc code does a bunch of locking which is unnecessary if
>> you have hardware which handles all of the queueing. Add
>> support for this, and skip over all of the queueing code if
>> the feature is enabled on a given device, which breaks QDisc
>> support on dpaa_eth, and also coopts the FCOE feature bit.
>>
>> Signed-off-by: Andy Fleming <[email protected]>
>> Signed-off-by: Ben Collins <[email protected]>
>
> Sorry, no.
>
> If we are going to support something like this then there needs to
> be full coordination, configuration wise, so that if we enable
> a qdisc that the hardware supports we submit it directly, but if
> we enable a qdisc the HW does not support, we still use the software
> qdisc.
>
> This also means that we need to have a way to determine if the qdisc
> configuration exceeds that parametorial limits of the device's HW
> capabilities, and fallback to software qdisc in those cases too.
It would appear one of our customers is attempting to upstream our code for us. We are aware that this current solution is unacceptable (which is why we have not submitted it), and we are currently trying to develop a less hacky solution that integrates with qdisc.
Andy
On Mar 22, 2013, at 10:23 AM, Eric Dumazet <[email protected]> wrote:
> On Wed, 2011-07-13 at 08:52 -0500, Andy Fleming wrote:
>> The QDisc code does a bunch of locking which is unnecessary if
>> you have hardware which handles all of the queueing. Add
>> support for this, and skip over all of the queueing code if
>> the feature is enabled on a given device, which breaks QDisc
>> support on dpaa_eth, and also coopts the FCOE feature bit.
>>
>> Signed-off-by: Andy Fleming <[email protected]>
>> Signed-off-by: Ben Collins <[email protected]>
>> Cc: [email protected]
>> ---
>> include/linux/netdev_features.h | 2 ++
>> net/core/dev.c | 6 ++++++
>> 2 files changed, 8 insertions(+)
>>
>> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
>> index 3dd3934..ffb4587 100644
>> --- a/include/linux/netdev_features.h
>> +++ b/include/linux/netdev_features.h
>> @@ -56,6 +56,7 @@ enum {
>> NETIF_F_LOOPBACK_BIT, /* Enable loopback */
>> NETIF_F_RXFCS_BIT, /* Append FCS to skb pkt data */
>> NETIF_F_RXALL_BIT, /* Receive errored frames too */
>> + NETIF_F_HW_QDISC_BIT, /* Supports hardware Qdisc */
>>
>> /*
>> * Add your fresh new feature above and remember to update
>> @@ -80,6 +81,7 @@ enum {
>> #define NETIF_F_GSO_ROBUST __NETIF_F(GSO_ROBUST)
>> #define NETIF_F_HIGHDMA __NETIF_F(HIGHDMA)
>> #define NETIF_F_HW_CSUM __NETIF_F(HW_CSUM)
>> +#define NETIF_F_HW_QDISC __NETIF_F(HW_QDISC)
>> #define NETIF_F_HW_VLAN_FILTER __NETIF_F(HW_VLAN_FILTER)
>> #define NETIF_F_HW_VLAN_RX __NETIF_F(HW_VLAN_RX)
>> #define NETIF_F_HW_VLAN_TX __NETIF_F(HW_VLAN_TX)
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index dffbef7..6818b18 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -2743,6 +2743,12 @@ int dev_queue_xmit(struct sk_buff *skb)
>>
>> skb_update_prio(skb);
>>
>> + if (dev->features & NETIF_F_HW_QDISC) {
>> + txq = netdev_pick_tx(dev, skb);
>> + rc = dev_hard_start_xmit(skb, dev, txq);
>> + goto out;
>> + }
>> +
>> txq = netdev_pick_tx(dev, skb);
>> q = rcu_dereference_bh(txq->qdisc);
>>
>
> Nobody forces you to use a qdisc.
>
> And if your device really is lockless, it can use NETIF_F_LLTX feature.
NETIF_F_LLTX_BIT, /* LockLess TX - deprecated. Please */
/* do not use LLTX in new drivers */
--
Servergy : http://www.servergy.com/
SwissDisk : http://www.swissdisk.com/
Ubuntu : http://www.ubuntu.com/
My Blog : http://ben-collins.blogspot.com/
On Fri, 2013-03-22 at 10:50 -0400, Ben Collins wrote:
>
> NETIF_F_LLTX_BIT, /* LockLess TX - deprecated. Please */
> /* do not use LLTX in new drivers */
Yes, but this is the current way to do that.
Unless you design a complete new layer to replace it.
From: Ben Collins <[email protected]>
Date: Fri, 22 Mar 2013 10:43:44 -0400
> "For us" is a loose term, when it's more that we are attempting to
> upstream code so our system is supported by a mainline kernel
> instead of having one-off kernels.
If this other person doesn't want their code upstreams, it is absolutely
inappropriate for you to try and force the matter and "do it for them."
On Mar 22, 2013, at 11:17 AM, David Miller <[email protected]> wrote:
> From: Ben Collins <[email protected]>
> Date: Fri, 22 Mar 2013 10:43:44 -0400
>
>> "For us" is a loose term, when it's more that we are attempting to
>> upstream code so our system is supported by a mainline kernel
>> instead of having one-off kernels.
>
> If this other person doesn't want their code upstreams, it is absolutely
> inappropriate for you to try and force the matter and "do it for them."
If your company had hardware going to production, you'd want it supported in mainline too, I suspect.
Either way, I'll redact this patch because it isn't necessarily needed for the driver to work.
--
Servergy : http://www.servergy.com/
SwissDisk : http://www.swissdisk.com/
Ubuntu : http://www.ubuntu.com/
My Blog : http://ben-collins.blogspot.com/
From: Ben Collins <[email protected]>
Date: Fri, 22 Mar 2013 11:39:20 -0400
> If your company had hardware going to production, you'd want it
> supported in mainline too, I suspect.
But never against the wishes of the author of the code.
This has firm and strict precedence, for example one of the
implementations block layer encryption was not wanted to be merged by
the author, and Linus reverted it.
So if the person who wrote the code doesn't want it upstream, you can't
bypass them against their wishes, ever. It's their code not your's.
On Mar 22, 2013, at 11:41 AM, David Miller <[email protected]> wrote:
> From: Ben Collins <[email protected]>
> Date: Fri, 22 Mar 2013 11:39:20 -0400
>
>> If your company had hardware going to production, you'd want it
>> supported in mainline too, I suspect.
>
> But never against the wishes of the author of the code.
>
> This has firm and strict precedence, for example one of the
> implementations block layer encryption was not wanted to be merged by
> the author, and Linus reverted it.
>
> So if the person who wrote the code doesn't want it upstream, you can't
> bypass them against their wishes, ever. It's their code not your's.
They never made such a statement. The commit (which is publicly available in Freescale's git repo) makes no mention of it being "good enough for our SDK, but don't send upstream". Freescale wants this upstream, but doesn't have the resources because they are embedded focused and gear more toward SDKs to support their SoCs (currently that means a 3.0.x kernel).
Don't accuse me of something I didn't do. Also, if there's a patch that makes my hardware work, but I can't use it because (even though it's open source licensed) the author doesn't want it in mainline, then that is effectively squatting.
We are a hardware company. We've been provided with a driver for the platform we designed by the SoC vendor. It's GPL licensed. We've attempted to get this done by them, but they haven't been able to, so we are exercising prudence and making sure our platform is supported in mainline. I don't see where that's any different than tons of other patches that go into the kernel.
If it makes everyone feel better, I'll limit attribution in the patches to just an Originally-By: line.
--
Servergy : http://www.servergy.com/
SwissDisk : http://www.swissdisk.com/
Ubuntu : http://www.ubuntu.com/
My Blog : http://ben-collins.blogspot.com/
From: Ben Collins <[email protected]>
Date: Fri, 22 Mar 2013 11:53:54 -0400
> Also, if there's a patch that makes my hardware work, but I can't
> use it because (even though it's open source licensed) the author
> doesn't want it in mainline, then that is effectively squatting.
This is called respecting the wishes of the author of the code, I'm
sorry if this is a foreign concept for you.
From: Ben Collins <[email protected]>
Date: Fri, 22 Mar 2013 12:14:44 -0400
> I'm sorry, I thought we were working on open source projects
> here. If the code isn't encumbered by patents, legal issues or
> technical problems, and is licensed compatible with the kernel, I
> just thought it was fair game.
This is about being nice to people, rather than exercising your rights
to the maximum possible extent regardless of how that effects other
people.
On Mar 22, 2013, at 11:59 AM, David Miller <[email protected]> wrote:
> From: Ben Collins <[email protected]>
> Date: Fri, 22 Mar 2013 11:53:54 -0400
>
>> Also, if there's a patch that makes my hardware work, but I can't
>> use it because (even though it's open source licensed) the author
>> doesn't want it in mainline, then that is effectively squatting.
>
> This is called respecting the wishes of the author of the code, I'm
> sorry if this is a foreign concept for you.
I'm sorry, I thought we were working on open source projects here. If the code isn't encumbered by patents, legal issues or technical problems, and is licensed compatible with the kernel, I just thought it was fair game.
--
Servergy : http://www.servergy.com/
SwissDisk : http://www.swissdisk.com/
Ubuntu : http://www.ubuntu.com/
My Blog : http://ben-collins.blogspot.com/
On Mar 22, 2013, at 12:16 PM, David Miller <[email protected]> wrote:
> From: Ben Collins <[email protected]>
> Date: Fri, 22 Mar 2013 12:14:44 -0400
>
>> I'm sorry, I thought we were working on open source projects
>> here. If the code isn't encumbered by patents, legal issues or
>> technical problems, and is licensed compatible with the kernel, I
>> just thought it was fair game.
>
> This is about being nice to people, rather than exercising your rights
> to the maximum possible extent regardless of how that effects other
> people.
Right, because a 3-line patch is such a ballsy move on my part. Better restrain myself next time.
--
Servergy : http://www.servergy.com/
SwissDisk : http://www.swissdisk.com/
Ubuntu : http://www.ubuntu.com/
My Blog : http://ben-collins.blogspot.com/
On Fri, Mar 22, 2013 at 11:39:20AM -0400, Ben Collins wrote:
>
> If your company had hardware going to production, you'd want it supported in mainline too, I suspect.
And if companies told their hardware partners that they will drop use
of their hardware in future products unless they get their !@#@S
drivers upstream, I'd bet they'd change their engineering priorities
so they would work on it, instead of foisting this work on their
customers.
I've seen this work in enterprise computing, where the RFP had
requirements for upstream drivers (i.e., if you want your 10gig
ethernet NIC to be used in HP or IBM's servers, get the darned thing
upstream!). The trick is making it clear that selection of components
depends not just on an OSS driver, but an OSS driver which has been
accepted upstream (which also helps from a quality-of-code
requirement).
I've been waiting for this to start happening in the consumer
electronics/embedded world, but it's been slow coming,
unfortunately....
- Ted
On Fri, Mar 22, 2013 at 10:43:44AM -0400, Ben Collins wrote:
> On Mar 22, 2013, at 10:33 AM, David Miller <[email protected]> wrote:
>
> > From: Fleming Andy-AFLEMING <[email protected]>
> > Date: Fri, 22 Mar 2013 14:31:50 +0000
> >
> >> It would appear one of our customers is attempting to upstream our
> >> code for us. We are aware that this current solution is unacceptable
> >> (which is why we have not submitted it), and we are currently trying
> >> to develop a less hacky solution that integrates with qdisc.
> >
> > Ben, can can you coordinate with people instead of doing crap like
> > this?
>
>
> "For us" is a loose term, when it's more that we are attempting to upstream code so our system is supported by a mainline kernel instead of having one-off kernels.
>
> And we have been talking with Freescale about this for quite some time (couple years?). They have a roadmap that doesn't include getting this driver supported in mainline any time soon, so I'm taking time to get this done for our own system. I'm not meaning to step on any toes.
>
How true. You are not the only one with thatt problem.
Ben, can you possibly send me your complete patch set on top of 3.8, or point me
to a git tree ?
I have exactly the same problem, I need the code in 3.8, it doesn't seem
like the Freescale code will show up anytime soon, and I don't seem to be able
to get early code for testing either. I can promise you a free test coverage
and bug fixing ...
Thanks,
Guenter
On Fri, Mar 22, 2013 at 06:08:40PM -0400, Theodore Ts'o wrote:
> On Fri, Mar 22, 2013 at 11:39:20AM -0400, Ben Collins wrote:
> >
> > If your company had hardware going to production, you'd want it supported in mainline too, I suspect.
>
> And if companies told their hardware partners that they will drop use
> of their hardware in future products unless they get their !@#@S
> drivers upstream, I'd bet they'd change their engineering priorities
> so they would work on it, instead of foisting this work on their
> customers.
>
I would love to be in that position. However, the decision to choose
a specific chip is not always coordinated with those who have to provide
the software to run on those chips.
> I've seen this work in enterprise computing, where the RFP had
> requirements for upstream drivers (i.e., if you want your 10gig
> ethernet NIC to be used in HP or IBM's servers, get the darned thing
> upstream!). The trick is making it clear that selection of components
> depends not just on an OSS driver, but an OSS driver which has been
> accepted upstream (which also helps from a quality-of-code
> requirement).
>
> I've been waiting for this to start happening in the consumer
> electronics/embedded world, but it's been slow coming,
> unfortunately....
>
The same applies to vendors of non-consumer network devices, unfortunately.
Guenter
On Sat, Mar 23, 2013 at 12:02:21PM -0700, Guenter Roeck wrote:
> On Fri, Mar 22, 2013 at 10:43:44AM -0400, Ben Collins wrote:
> > On Mar 22, 2013, at 10:33 AM, David Miller <[email protected]> wrote:
> >
> > > From: Fleming Andy-AFLEMING <[email protected]>
> > > Date: Fri, 22 Mar 2013 14:31:50 +0000
> > >
> > >> It would appear one of our customers is attempting to upstream our
> > >> code for us. We are aware that this current solution is unacceptable
> > >> (which is why we have not submitted it), and we are currently trying
> > >> to develop a less hacky solution that integrates with qdisc.
> > >
> > > Ben, can can you coordinate with people instead of doing crap like
> > > this?
> >
> >
> > "For us" is a loose term, when it's more that we are attempting to upstream code so our system is supported by a mainline kernel instead of having one-off kernels.
> >
> > And we have been talking with Freescale about this for quite some time (couple years?). They have a roadmap that doesn't include getting this driver supported in mainline any time soon, so I'm taking time to get this done for our own system. I'm not meaning to step on any toes.
> >
> How true. You are not the only one with thatt problem.
>
> Ben, can you possibly send me your complete patch set on top of 3.8, or point me
> to a git tree ?
>
Never mind, I found your tree on github.
I'll clone it next week and start playing with it. That is going to save me a
lot of time. Thanks a lot for the work!
Our system is based on P5040, and I might be able to do some testing on P5020
as well. I'll let you know how it goes.
Thanks,
Guenter
On Fri, Mar 22, 2013 at 06:08:40PM -0400, Theodore Ts'o wrote:
[Insisting on upstream drivers]
> I've been waiting for this to start happening in the consumer
> electronics/embedded world, but it's been slow coming,
> unfortunately....
For CE at least it's not really relevant as nobody's got much intention
of upgrading the kernel on a shipping device, other than code review
there's not much benefit to the system integrator and that's generally
tilting at windmills a bit.