* RE: [PATCH net-next] core: Simplify logic in dev_load().
From: David Laight @ 2014-09-03 9:04 UTC (permalink / raw)
To: 'Sergei Shtylyov', Linux Netdev List, David Miller
In-Reply-To: <5406033C.40507@cogentembedded.com>
> > The code in dev_load() is convoluted.
> > Return early and remove the 'no_module' variable.
>
> You forgot to sign off on the patch, so it can't be applied.
Gah, git defaults and brain-fade.
Does that mean that if I submit a patch without a sign-off then no one
else is allowed to submit the same patch?
I see a DoS attack :-)
David
^ permalink raw reply
* RE: [PATCH net] core: Don't attempt to load the "" driver.
From: David Laight @ 2014-09-03 9:02 UTC (permalink / raw)
To: 'Cong Wang'; +Cc: Linux Netdev List, David Miller
In-Reply-To: <CAHA+R7P8=pkYfSoqhkiAkLc03V38_WCbqJdLxQKF_Y6YXNnbeg@mail.gmail.com>
> On Tue, Sep 2, 2014 at 6:48 AM, David Laight <David.Laight@aculab.com> wrote:
> > While the applications shouldn't be calling an SIOCxxx ioctl with ifr_name[0] == 0
> > the kernel shouldn't be tracing the error either.
> >
>
> Why don't we reject this empty string? It doesn't look like a valid one.
> I assume this is for compatibility?
The ioctl code will error it later on - the module load is 'speculative'.
Analysing whether all the ioctls need dev_load() to succeed is another issue.
Indeed I'm not sure anything stops the module being unloaded before the
ioctl action tries to take a real reference on the interface.
Whether request_module("") should be an error is a different question,
probably much harder to analyse.
David
^ permalink raw reply
* [PATCH V2 net-next] core: Simplify logic in dev_load().
From: David Laight @ 2014-09-03 8:57 UTC (permalink / raw)
To: 'Linux Netdev List', 'David Miller'
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D174856D6@AcuExch.aculab.com>
The code in dev_load() is convoluted.
Return early and remove the 'no_module' variable.
Signed-off-by: David Laight <david.laight@aculab.com>
---
V2: Added signed-off
Note that this patch will only apply after the one that adds the
check for name[0].
net/core/dev_ioctl.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 84edf16..4b7ab54 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -356,7 +356,6 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
void dev_load(struct net *net, const char *name)
{
struct net_device *dev;
- int no_module;
if (!name[0])
return;
@@ -364,15 +363,15 @@ void dev_load(struct net *net, const char *name)
rcu_read_lock();
dev = dev_get_by_name_rcu(net, name);
rcu_read_unlock();
+ if (dev)
+ return;
- no_module = !dev;
- if (no_module && capable(CAP_NET_ADMIN))
- no_module = request_module("netdev-%s", name);
- if (no_module && capable(CAP_SYS_MODULE)) {
- if (!request_module("%s", name))
- pr_warn("Loading kernel module for a network device with CAP_SYS_MODULE (deprecated). Use CAP_NET_ADMIN and alias netdev-%s instead.\n",
- name);
- }
+ if (capable(CAP_NET_ADMIN) && !request_module("netdev-%s", name))
+ return;
+
+ if (capable(CAP_SYS_MODULE) && !request_module("%s", name))
+ pr_warn("Loading kernel module for a network device with CAP_SYS_MODULE (deprecated). Use CAP_NET_ADMIN and alias netdev-%s instead.\n",
+ name);
}
EXPORT_SYMBOL(dev_load);
--
1.8.1.2
^ permalink raw reply related
* [PATCH V2 net] core: Don't attempt to load the "" driver.
From: David Laight @ 2014-09-03 8:55 UTC (permalink / raw)
To: 'Linux Netdev List', 'David Miller'
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D174856B9@AcuExch.aculab.com>
Requests like 'ifconfig ""' cause dev_load(net, "") be called, since
request_module("") return 0 (success) this generates the error message:
"Loading kernel module for a network device with CAP_SYS_MODULE (deprecated). Use CAP_NET_ADMIN and alias netdev- instead."
Since dev_load() doesn't have to work, just ignore such names.
Signed-off-by: David Laight <david.laight@aculab.com>
---
V2: Added sign off.
If you search for the above error message, you'll find a lot of complaints.
While the applications shouldn't be calling an SIOCxxx ioctl with ifr_name[0] == 0
the kernel shouldn't be tracing the error either.
Due to the complaints from users, this might be a backport candidate.
net/core/dev_ioctl.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index cf999e0..84edf16 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -358,6 +358,9 @@ void dev_load(struct net *net, const char *name)
struct net_device *dev;
int no_module;
+ if (!name[0])
+ return;
+
rcu_read_lock();
dev = dev_get_by_name_rcu(net, name);
rcu_read_unlock();
--
1.8.1.2
^ permalink raw reply related
* Re: [PATCH] net: export pkt_type_offset() helper
From: Denis Kirjanov @ 2014-09-03 8:53 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Denis Kirjanov, netdev, Markos Chandras, Martin Schwidefsky,
Daniel Borkmann
In-Reply-To: <20140902225926.GA14635@its-macbook-pro.plumgrid.com>
On 9/3/14, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> On Wed, Sep 03, 2014 at 12:03:58AM +0400, Denis Kirjanov wrote:
>> Currently we have 2 pkt_type_offset functions doing
>> the same thing and spread across the architecture files.
>> Let's use the generic helper routine.
>
> that would be good cleanup.
> Please tag your subject as '[PATCH net-next]...'
>
>> Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
>> Cc: Markos Chandras <markos.chandras@imgtec.com>
>> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
>> Cc: Daniel Borkmann <dborkman@redhat.com>
>> ---
>> arch/mips/net/bpf_jit.c | 21 ---------------------
>> arch/s390/net/bpf_jit_comp.c | 38 ++++----------------------------------
>> include/linux/filter.h | 7 +++++++
>> net/core/filter.c | 7 +------
>> 4 files changed, 12 insertions(+), 61 deletions(-)
>>
>> diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
>> -#define PKT_TYPE_MAX 0xe0
>> -static int pkt_type_offset;
> ...
>> -}
>> -device_initcall(bpf_pkt_type_offset_init);
>
> may be make sense to optimize common case via initcall() as well
> instead of dropping it for s390?
Yeah, it's a better option.
>
>> diff --git a/include/linux/filter.h b/include/linux/filter.h
>> index a5227ab..97e0549 100644
>> --- a/include/linux/filter.h
>> +++ b/include/linux/filter.h
>> @@ -424,6 +424,13 @@ static inline void *bpf_load_pointer(const struct
>> sk_buff *skb, int k,
>> return bpf_internal_load_pointer_neg_helper(skb, k, size);
>> }
>>
>> +#ifdef __BIG_ENDIAN_BITFIELD
>> +#define PKT_TYPE_MAX (7 << 5)
>> +#else
>> +#define PKT_TYPE_MAX 7
>> +#endif
>
> these defines don't need to move. keep them in filter.c
I'll respin. Thanks!
>
> --
> 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
>
--
Regards,
Denis
^ permalink raw reply
* Re: [PATCH net-next v2 2/2] r8152: use eth_hw_addr_random
From: Bjørn Mork @ 2014-09-03 8:33 UTC (permalink / raw)
To: Hayes Wang
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, nic_swsd-Rasf1IRRPZFBDgjK7y7TUQ,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1394712342-15778-31-Taiwan-albertk-Rasf1IRRPZFBDgjK7y7TUQ@public.gmane.org>
Hayes Wang <hayeswang-Rasf1IRRPZFBDgjK7y7TUQ@public.gmane.org> writes:
> + ether_addr_copy(dev->perm_addr, dev->dev_addr);
This isn't strictly necessary. register_netdev() will do it for you as
long as you leave the default addr_assign_type (NET_ADDR_PERM). And you
do.
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 3/6] drivers: net: ethernet: cpsw: add multicast address to ALE table
From: Mugunthan V N @ 2014-09-03 8:28 UTC (permalink / raw)
To: Graeme Smecher, netdev
In-Reply-To: <1409682156.3334.16.camel@fromme.threespeedlogic.local>
Hi Graeme
There is already a support for add multicast in v2.6.37 cpsw driver, if
there is a bug and you found a fix, can go a head and fix that. The
patch you have mentioned is an upstream patch with was done on top of
am335x platform, so there might be bugs fixed and added into the patch
which are not in v2.6.37 cpsw driver.
Regards
Mugunthan V N
On Tuesday 02 September 2014 11:52 PM, Graeme Smecher wrote:
> Hi Mugunthan,
>
>> On Wed, Oct 17, 2012 at 04:15:15AM +0530, Mugunthan V N wrote:
>>> Adding multicast address to ALE table via netdev ops to subscribe, transmit
>>> or receive multicast frames to and from the network
>>
>> Is this somehow related to the time stamping function? If so, how?
>>
>> Thanks,
>> Richard
>
> Can you give me a brief description of this (relatively ancient) patch? Your
> original e-mail is visible here:
>
> http://marc.info/?l=linux-netdev&m=135042754927177&w=2
>
> I'm wondering if this patch needs to be backported to older CPSW driver
> snapshots (specifically, TI's 2.6.37 branch for dm81xx.) It appears to
> fix a multicast bug I'm tracking down, but it's difficult to know for
> sure without a good description of what problem the patch addresses. (I
> don't want to commit code that fixes my hardware by accident.)
>
> For a little more information, you can refer to my e2e.ti.com post:
>
> http://e2e.ti.com/support/dsp/davinci_digital_media_processors/f/716/t/365586.aspx
>
> I'm slowly learning about ALE filtering, but a quick reply from a
> domain expert would be very helpful.
>
> best,
> Graeme
>
^ permalink raw reply
* Re: [PATCH 2/2] net: ethernet: cpsw: fix interrupt lookup logic in cpsw_probe()
From: Daniel Mack @ 2014-09-03 8:23 UTC (permalink / raw)
To: Mugunthan V N, davem; +Cc: julia.lawall, netdev, george.cherian
In-Reply-To: <5406CFD9.6060706@ti.com>
On 09/03/2014 10:22 AM, Mugunthan V N wrote:
> On Wednesday 03 September 2014 01:00 PM, Daniel Mack wrote:
>> On 09/03/2014 09:28 AM, Mugunthan V N wrote:
>>> On Tuesday 02 September 2014 10:14 PM, Daniel Mack wrote:
>>>> The code in cpsw_probe() currently iterates over the available
>>>> interrupt resources and requests each of them. While doing so, it
>>>> keeps track of their indices through priv->irqs_table.
>>>>
>>>> However, the code currently only remembers the last interrupt in
>>>> a resource, and will leak the others if there is more than one.
>>>> This can only happen for board-file driven platforms and not via DT,
>>>> however.
>>>>
>>>> Also, there is currently no bounds check, while priv->irqs_table is a
>>>> fixed-size array. If we are passed more than 4 resources, we're in
>>>> trouble.
>>>>
>>>> This patch introduces a bounds check and changes the way interrupt
>>>> indices are kept. Tested on a Beagle Bone Black board only.
>>>>
>>>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>>>
>>> The drivers is not supported for non-DT platforms as all the platforms
>>> which uses CPSW are DT only platforms.
>>
>> Ok, thanks for explaining.
>>
>> But then we can remove the iteration then and simplify the code, right?
>> The bounds check should also be done.
>>
>
> Right, we can simplify the code.
Ok, I'll cook up a new patch.
Thanks!
Daniel
^ permalink raw reply
* Re: [PATCH 2/2] net: ethernet: cpsw: fix interrupt lookup logic in cpsw_probe()
From: Mugunthan V N @ 2014-09-03 8:22 UTC (permalink / raw)
To: Daniel Mack, davem; +Cc: julia.lawall, netdev, george.cherian
In-Reply-To: <5406C3A2.4060103@zonque.org>
On Wednesday 03 September 2014 01:00 PM, Daniel Mack wrote:
> On 09/03/2014 09:28 AM, Mugunthan V N wrote:
>> On Tuesday 02 September 2014 10:14 PM, Daniel Mack wrote:
>>> The code in cpsw_probe() currently iterates over the available
>>> interrupt resources and requests each of them. While doing so, it
>>> keeps track of their indices through priv->irqs_table.
>>>
>>> However, the code currently only remembers the last interrupt in
>>> a resource, and will leak the others if there is more than one.
>>> This can only happen for board-file driven platforms and not via DT,
>>> however.
>>>
>>> Also, there is currently no bounds check, while priv->irqs_table is a
>>> fixed-size array. If we are passed more than 4 resources, we're in
>>> trouble.
>>>
>>> This patch introduces a bounds check and changes the way interrupt
>>> indices are kept. Tested on a Beagle Bone Black board only.
>>>
>>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>>
>> The drivers is not supported for non-DT platforms as all the platforms
>> which uses CPSW are DT only platforms.
>
> Ok, thanks for explaining.
>
> But then we can remove the iteration then and simplify the code, right?
> The bounds check should also be done.
>
Right, we can simplify the code.
Regards
Mugunthan V N
^ permalink raw reply
* Re: [PATCH net-next 2/2] net: exit busy loop when another process is runnable
From: Michael S. Tsirkin @ 2014-09-03 8:09 UTC (permalink / raw)
To: Ingo Molnar
Cc: Mike Galbraith, Jason Wang, davem, netdev, linux-kernel,
Peter Zijlstra, Ingo Molnar, Eliezer Tamir
In-Reply-To: <20140822073653.GA7372@gmail.com>
On Fri, Aug 22, 2014 at 09:36:53AM +0200, Ingo Molnar wrote:
>
> > > diff --git a/include/net/busy_poll.h b/include/net/busy_poll.h
> > > index 1d67fb6..8a33fb2 100644
> > > --- a/include/net/busy_poll.h
> > > +++ b/include/net/busy_poll.h
> > > @@ -109,7 +109,8 @@ static inline bool sk_busy_loop(struct sock *sk, int nonblock)
> > > cpu_relax();
> > >
> > > } while (!nonblock && skb_queue_empty(&sk->sk_receive_queue) &&
> > > - !need_resched() && !busy_loop_timeout(end_time));
> > > + !need_resched() && !busy_loop_timeout(end_time) &&
> > > + nr_running_this_cpu() < 2);
>
> So it's generally a bad idea to couple to the scheduler through
> such a low level, implementation dependent value like
> 'nr_running', causing various problems:
>
> - It misses important work that might be pending on this CPU,
> like RCU callbacks.
>
> - It will also over-credit task contexts that might be
> runnable, but which are less important than the currently
> running one: such as a SCHED_IDLE task
>
> - It will also over-credit even regular SCHED_NORMAL tasks, if
> this current task is more important than them: say
> SCHED_FIFO. A SCHED_FIFO workload should run just as fast
> with SCHED_NORMAL tasks around, as a SCHED_NORMAL workload
> on an otherwise idle system.
>
> So what you want is a more sophisticated query to the
> scheduler, a sched_expected_runtime() method that returns the
> number of nsecs this task is expected to run in the future,
> which returns 0 if you will be scheduled away on the next
> schedule(), and returns infinity for a high prio SCHED_FIFO
> task, or if this SCHED_NORMAL task is on an otherwise idle CPU.
>
> It will return a regular time slice value in other cases, when
> there's some load on the CPU.
>
> The polling logic can then do its decision based on that time
> value.
>
> All this can be done reasonably fast and lockless in most
> cases, so that it can be called from busy-polling code.
>
> An added advantage would be that this approach consolidates the
> somewhat random need_resched() checks into this method as well.
>
> In any case I don't agree with the nr_running_this_cpu()
> method.
>
> (Please Cc: me and lkml to future iterations of this patchset.)
>
> Thanks,
>
> Ingo
This sounds very nice.
We could then have applications say "I want to poll
only if no one else has urgent work" where
urgent is defined as "has to run within next N nseconds".
Really just a bit more flexibility added to busy polling.
Peter, Ingo, does this sound good?
--
MST
^ permalink raw reply
* RE: [PATCH net-next 4/4] ipv6: coding style - cleanse bracing
From: Yuval Mintz @ 2014-09-03 7:52 UTC (permalink / raw)
To: YOSHIFUJI Hideaki/吉藤英明, Ian Morris,
netdev
Cc: YOSHIFUJI Hideaki
In-Reply-To: <54066F6D.4060406@miraclelinux.com>
> Hi,
>
> Ian Morris wrote:
> > Tidy up braces in a few places (mainly if statements):
> > * remove unnecessary braces
> > * add braces for single line statements where part of an "else" where
> > the other branch has braces
>
> Hmm, I am not for this change.
>
> If "if" (or "else") statement occupies multiple lines, I want to see braces even if it
> contains a single statement.
> On the other hand, especiallly if the statement occupiles only one line (and the
> logic do not require those braces), no braces.
>
> e.g. 1
> if (cond)
> statement();
> else {
> statement2(argument,
> argument2);
> }
I was under the impression the coding style require matching curly braces on all
if and else clauses, e.g., in the previous example I would have expected the `if' to
have curly braces since its else block has them, regardless of whether its
statement-block has more than a single line.
> e.g. 2
> if (cond) {
> if (cond2)
> statement();
> } else
> statement2(argument, argument2);
>
> e.g. 3
> if (cond) {
> /* comment */
> statement();
> }
________________________________
This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.
^ permalink raw reply
* Re: [PATCH net-next 2/2] net: exit busy loop when another process is runnable
From: Michael S. Tsirkin @ 2014-09-03 7:51 UTC (permalink / raw)
To: Eliezer Tamir
Cc: Jason Wang, Ingo Molnar, Mike Galbraith, davem, netdev,
linux-kernel, Peter Zijlstra, Ingo Molnar
In-Reply-To: <5406B9E6.6050103@linux.intel.com>
On Wed, Sep 03, 2014 at 09:49:10AM +0300, Eliezer Tamir wrote:
> On 02/09/2014 11:31, Michael S. Tsirkin wrote:
> > On Tue, Sep 02, 2014 at 09:15:18AM +0300, Eliezer Tamir wrote:
> >> On 02/09/2014 06:29, Jason Wang wrote:
> >>> On 09/01/2014 02:39 PM, Eliezer Tamir wrote:
> >>>> On 29/08/2014 06:08, Jason Wang wrote:
> >>>>>> Yes, but rx busy polling only works in process context and does not
> >>>>>> disable bh, so it may be not an issue.
> >>>> sk_busy_loop() uses rcu_read_lock_bh(), so it does run with bh disabled.
> >>>
> >>> True, so we need probably also exit the loop when there are pending bhs.
> >>
> >> I'm not so sure, in the typical busy poll scenario, the incoming
> >> traffic is the most time-critical thing in the system.
> >> It's so important that you are willing to trade lots of CPU power
> >> for better latency. The user has decided that he wants to dedicate
> >> this CPU mostly for that. This is not something that plays nice with
> >> other apps, but this is what the user wants.
> >
> > I think most applications wouldn't interpret this flag as "burn up CPU I don't
> > care what is the result", what apps want is more of "maximise throughput
> > and minimise latency even if throughput/CPU ratio goes down".
> > Jason posted benchmarks that show throughput going up because other
> > processes get more of a chance to run, so this seems consistent
> > with that goal.
>
> Busy polling is not a general purpose feature, it's not something you
> can casually turn on and will "just work". Most applications should not
> be using busy polling. Currently it is used by multiserver applications
> that you spend days tuning to specific platforms.
>
> What the user wants is to lower both avg and maximum latencies, at the
> expense of everything else including power efficiency and sometimes
> even throughput. The only exception is making the system crash ;)
>
> While letting other things take precedence over busy polling might not
> hurt the avg latency much, it will kill your maximum latency.
>
> -Eliezer
If scheduler happens to run both server and client on the
same CPU, polling will hurt maximum latency even more.
So I guess different users want different things.
How about applications giving us a hint what they prefer?
For example, a new flag that says "I don't have anything useful to do so
let's do busy polling but my server is on the local system, so please
only poll if CPU is otherwise idle".
--
MST
^ permalink raw reply
* Re: [PATCH net-next 2/2] net: exit busy loop when another process is runnable
From: Jason Wang @ 2014-09-03 7:33 UTC (permalink / raw)
To: Eliezer Tamir, Michael S. Tsirkin
Cc: Ingo Molnar, Mike Galbraith, davem, netdev, linux-kernel,
Peter Zijlstra, Ingo Molnar
In-Reply-To: <5406B9E6.6050103@linux.intel.com>
On 09/03/2014 02:49 PM, Eliezer Tamir wrote:
> On 02/09/2014 11:31, Michael S. Tsirkin wrote:
>> On Tue, Sep 02, 2014 at 09:15:18AM +0300, Eliezer Tamir wrote:
>>> On 02/09/2014 06:29, Jason Wang wrote:
>>>> On 09/01/2014 02:39 PM, Eliezer Tamir wrote:
>>>>> On 29/08/2014 06:08, Jason Wang wrote:
>>>>>>> Yes, but rx busy polling only works in process context and does not
>>>>>>> disable bh, so it may be not an issue.
>>>>> sk_busy_loop() uses rcu_read_lock_bh(), so it does run with bh disabled.
>>>> True, so we need probably also exit the loop when there are pending bhs.
>>> I'm not so sure, in the typical busy poll scenario, the incoming
>>> traffic is the most time-critical thing in the system.
>>> It's so important that you are willing to trade lots of CPU power
>>> for better latency. The user has decided that he wants to dedicate
>>> this CPU mostly for that. This is not something that plays nice with
>>> other apps, but this is what the user wants.
>> I think most applications wouldn't interpret this flag as "burn up CPU I don't
>> care what is the result", what apps want is more of "maximise throughput
>> and minimise latency even if throughput/CPU ratio goes down".
>> Jason posted benchmarks that show throughput going up because other
>> processes get more of a chance to run, so this seems consistent
>> with that goal.
> Busy polling is not a general purpose feature, it's not something you
> can casually turn on and will "just work". Most applications should not
> be using busy polling.
How about busy read? It was enabled only through a global sysctl and
then applications can make use of this without rewrite.
> Currently it is used by multiserver applications
> that you spend days tuning to specific platforms.
I agree the polling code needs more thought but the patch only change
busy read.
A new issue is for virt users. I implement busy polling for virtio-net
but we don't want one vcpu monopoly the cpu if there's some tasks on
other vcpus. We may need some hint from host to guest to let it exit the
loop if needed.
>
> What the user wants is to lower both avg and maximum latencies, at the
> expense of everything else including power efficiency and sometimes
> even throughput. The only exception is making the system crash ;)
>
> While letting other things take precedence over busy polling might not
> hurt the avg latency much, it will kill your maximum latency.
>
> -Eliezer
^ permalink raw reply
* Re: iproute2 util: Change signature of rtnl dump filters to use struct params
From: Vadim Kochan @ 2014-09-03 7:32 UTC (permalink / raw)
To: netdev
In-Reply-To: <CAMw6YJJq1MP+r6=8KrhBi9bPUS8E1JJjb3qOjbjVf4Ano0AVsA@mail.gmail.com>
Sorry, this it not patch for probably applying, it was get into
patchwork queue after I copy pasted
here the diff, this is only for discussing.
Regards,
On Wed, Sep 3, 2014 at 2:46 AM, Vadim Kochan <vadim4j@gmail.com> wrote:
> Hi all,
>
> I am thinking about to make some refactoring and change the signature
> of all rtnl dump filters to form:
>
> diff --git a/include/libnetlink.h b/include/libnetlink.h
> index fe7d5d3..9514e67 100644
> --- a/include/libnetlink.h
> +++ b/include/libnetlink.h
> @@ -39,8 +39,14 @@ extern int rtnl_dump_request(struct rtnl_handle
> *rth, int type, void *req,
> int len)
> __attribute__((warn_unused_result));
>
> -typedef int (*rtnl_filter_t)(const struct sockaddr_nl *,
> - struct nlmsghdr *n, void *);
> +struct nl_param
> +{
> + struct sockaddr_nl *who;
> + struct nlmsghdr *n;
> + void *arg;
> +};
> +
> +typedef int (*rtnl_filter_t)(struct nl_param *p);
>
> struct rtnl_dump_filter_arg
> {
>
> What do you think?
>
> Regards,
^ permalink raw reply
* Re: [PATCH 2/2] net: ethernet: cpsw: fix interrupt lookup logic in cpsw_probe()
From: Daniel Mack @ 2014-09-03 7:30 UTC (permalink / raw)
To: Mugunthan V N, davem; +Cc: julia.lawall, netdev, george.cherian
In-Reply-To: <5406C336.3020005@ti.com>
On 09/03/2014 09:28 AM, Mugunthan V N wrote:
> On Tuesday 02 September 2014 10:14 PM, Daniel Mack wrote:
>> The code in cpsw_probe() currently iterates over the available
>> interrupt resources and requests each of them. While doing so, it
>> keeps track of their indices through priv->irqs_table.
>>
>> However, the code currently only remembers the last interrupt in
>> a resource, and will leak the others if there is more than one.
>> This can only happen for board-file driven platforms and not via DT,
>> however.
>>
>> Also, there is currently no bounds check, while priv->irqs_table is a
>> fixed-size array. If we are passed more than 4 resources, we're in
>> trouble.
>>
>> This patch introduces a bounds check and changes the way interrupt
>> indices are kept. Tested on a Beagle Bone Black board only.
>>
>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>
> The drivers is not supported for non-DT platforms as all the platforms
> which uses CPSW are DT only platforms.
Ok, thanks for explaining.
But then we can remove the iteration then and simplify the code, right?
The bounds check should also be done.
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH 2/2] net: ethernet: cpsw: fix interrupt lookup logic in cpsw_probe()
From: Mugunthan V N @ 2014-09-03 7:28 UTC (permalink / raw)
To: Daniel Mack, davem; +Cc: julia.lawall, netdev, george.cherian
In-Reply-To: <1409676245-13897-2-git-send-email-zonque@gmail.com>
On Tuesday 02 September 2014 10:14 PM, Daniel Mack wrote:
> The code in cpsw_probe() currently iterates over the available
> interrupt resources and requests each of them. While doing so, it
> keeps track of their indices through priv->irqs_table.
>
> However, the code currently only remembers the last interrupt in
> a resource, and will leak the others if there is more than one.
> This can only happen for board-file driven platforms and not via DT,
> however.
>
> Also, there is currently no bounds check, while priv->irqs_table is a
> fixed-size array. If we are passed more than 4 resources, we're in
> trouble.
>
> This patch introduces a bounds check and changes the way interrupt
> indices are kept. Tested on a Beagle Bone Black board only.
>
> Signed-off-by: Daniel Mack <zonque@gmail.com>
The drivers is not supported for non-DT platforms as all the platforms
which uses CPSW are DT only platforms.
Regards
Mugunthan V N
^ permalink raw reply
* Re: [PATCH 1/2] net: ethernet: cpsw: don't claim IRQs with devm_request_irq()
From: Mugunthan V N @ 2014-09-03 7:27 UTC (permalink / raw)
To: Daniel Mack, davem; +Cc: julia.lawall, netdev, george.cherian
In-Reply-To: <1409676245-13897-1-git-send-email-zonque@gmail.com>
On Tuesday 02 September 2014 10:14 PM, Daniel Mack wrote:
> Julia Lawall spotted a problem with aa1a15e ("net: ethernet: cpsw:
> switch to devres allocations") which introduced a race condition in
> cpsw_probe() by removing explicit interrupt disable calls before
> calling free_netdev(). Hence, an interrupt can fire after free_netdev()
> was called. The same problem exists in cpsw_remove().
>
> Fix this by reverting the IRQ part of the aforementioned patch and
> handle those resources explicitly.
>
> Reported-by: Julia Lawall <julia.lawall@lip6.fr>
> Signed-off-by: Daniel Mack <zonque@gmail.com>
CPSW interrupts cannot be triggered as the interrupts are disabled in
priv->wr_regs->tx_en and priv->wr_regs->rx_en inside CPSW module and
these interrupts are enabled only when the device is opened.
In cpsw_remove, CPDMA controller is stopped and interrupts are disabled
in cpsw_ndo_stop(), so there is no chance that an interrupt can occur
during cpsw_remove().
Regards
Mugunthan V N
^ permalink raw reply
* Re: [net PATCH 1/1] drivers: net: cpsw: dual_emac: fix reducing of rx descriptor during ifdown
From: Mugunthan V N @ 2014-09-03 7:16 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20140902.115434.1276391011837651390.davem@davemloft.net>
David
On Wednesday 03 September 2014 12:24 AM, David Miller wrote:
> From: Mugunthan V N <mugunthanvnm@ti.com>
> Date: Tue, 2 Sep 2014 14:51:05 +0530
>
>> When ifup and ifdown is run continuously, for each spilled packet (for
>> interface which is down) from DMA, the total number of rx descriptor
>> goes down and at one instance all the descriptor is lost and both the
>> interface stops working.
>>
>> To recover from this we need to put down both the interface and open the
>> interface which will re-init the DMA which intern queues fresh set of
>> skbs for rx.
>
> But you still should not receive packets for a netdev which is down.
> As far as I can tell, you're feeding it into the stack still.
If there is separate DMA for each netdev, then it is true that there
won't be a packet for a closed netdev. But in CPSW dual EMAC case, one
DMA engine is shared between two slave ports, so when one slave netdev
is put down, the DMA is not teared down as the other slave netdev is
still active. In heavy traffic network when putting down netdev, there
are chances that packets are already processed by DMA, and waiting for
NAPI to be submitted to network stack might belong to the netdev which
is already down.
So instead of just freeing the skb received on netdev which is already
down (which will reduce the total rx descriptors in rx dma channel),
requeue it back to DMA and make sure that rx dma descriptor count is
never reduced.
>
> Also this doesn't explain why the "status < 0" case applies to this
> new logic, you have not explained that at all.
>
This scenario is not generated when "status < 0", "status < 0" happens
when DMA is in tear-down mode, the above scenario happens when
netif_running(ndev) is false which denotes that netdev is down already.
Regards
Mugunthan V N
^ permalink raw reply
* ipvs ipv6 tunnel forwarding sets expires on local route
From: Alex Gartrell @ 2014-09-03 7:03 UTC (permalink / raw)
To: netdev; +Cc: lvs-devel, kernel-team, ps, traffic
So we've been debugging a problem for a while in 3.10 stable (we're
currently upgrading from 3.2) and it appears that we're expiring and
ultimately garbage collecting the local route for an ip we're adding to
the loopback device, resulting in ICMPV6_NOROUTE errors for clients.
I'd like your advice on how to fix this.
Repro:
"""
ipvsadm -R <<EOF
-A -t [face::1]:15213 -s ch
-a -t [face::1]:15213 -r 2401:db00:20:c001:face:0:45:0 -i
EOF
ip addr add dev lo face::1/128 || true
"""
This simply sets up a v6 service in ipvs and adds a real server ip which
is tunneling over ipip. This is important because this bug only affects
tunneling mode.
Within ipvs, in tunneling mode, we do the following when checking the
MTU in tunneling mode
/* MTU checking */
if (likely(!(rt_mode & IP_VS_RT_MODE_TUNNEL)))
mtu = dst_mtu(&rt->dst);
else {
struct sock *sk = skb->sk;
mtu = dst_mtu(&rt->dst) - sizeof(struct ipv6hdr);
if (mtu < IPV6_MIN_MTU) {
IP_VS_DBG_RL("%s(): mtu less than %d\n", __func__,
IPV6_MIN_MTU);
goto err_put;
}
ort = (struct rt6_info *) skb_dst(skb);
old_flags = ort->rt6i_flags;
if (!skb->dev && sk && sk->sk_state != TCP_TIME_WAIT)
ort->dst.ops->update_pmtu(&ort->dst, sk, NULL,
mtu);
}
So if there's a socket associated with the skb and it's not in
TCP_TIME_WAIT, we'll invoke the update_pmtu to ensure we generate
appropriately sized packets.
commit 81aded2 "ipv6: Handle PMTU in ICMP error handlers" introduces the
following.
@@ -1058,9 +1061,39 @@ static void ip6_rt_update_pmtu(struct dst_entry
*dst, u32 mtu)
dst_metric_set(dst, RTAX_FEATURES, features);
}
dst_metric_set(dst, RTAX_MTU, mtu);
+ rt6_update_expires(rt6, net->ipv6.sysctl.ip6_rt_mtu_expires);
}
}
The net result is that we end up setting an expiry on the local route.
When we hit ip6_rt_mtu_expires, the route expires (and is later GC'ed).
From that point forward we start ICMPV6_NOROUTE'ing packets in
ip6_rcv_finish until the address is removed and reinstalled.
I've got a couple of (bad?) ideas on how to fix it. We could simply
check rt6i_flags for (RTF_EXPIRES | RTF_CACHE) before setting expires.
We could also check for RTF_LOCAL. Alternatively, cloning the rt and
updating that might be an appropriate thing to do (in case the
tunneled-to route increases its MTU). I'm *completely* open to
suggestions :)
Thank you for your help,
--
Alex Gartrell <agartrell@fb.com>
^ permalink raw reply
* Re: [PATCH net-next 2/2] net: exit busy loop when another process is runnable
From: Jason Wang @ 2014-09-03 6:59 UTC (permalink / raw)
To: Eliezer Tamir, Eric Dumazet
Cc: Ingo Molnar, Mike Galbraith, davem, netdev, linux-kernel, mst,
Peter Zijlstra, Ingo Molnar jacob.e.keller@intel.com
In-Reply-To: <5406B365.5040604@linux.intel.com>
On 09/03/2014 02:21 PM, Eliezer Tamir wrote:
> On 02/09/2014 09:31, Jason Wang wrote:
>> On 09/02/2014 02:03 PM, Eliezer Tamir wrote:
>>> On 02/09/2014 06:35, Jason Wang wrote:
>>>> Not sure whether this method will scale considering thousands of sockets
>>>> and processes.
>>> There may be millions of sockets, but in most cases only a handful of
>>> device queues per CPU to busy poll on. I have tested the epoll rfc
>>> code with hundreds of thousands of sockets and one or two device
>>> queues and is scales pretty well.
>>>
>>> The part I don't like in that code is the cumbersome mechanism I used
>>> to track the socket -> queue relationship. I think that if I had more
>>> time to work on it, I would instead look into extending the epoll
>>> interface so that libevent can tell the kernel what it wants, instead
>>> of having the busypoll code try and learn it.
>> I'd like to have a look at this rfc. Could you please give me a pointer?
>> I've done a quick search on kernel mailing list but didn't find it.
> https://lkml.org/lkml/2013/8/21/192
>
> Cheers,
> Eliezer
> --
Thanks. I will have a look at this series.
^ permalink raw reply
* Re: [PATCH net-next 2/2] net: exit busy loop when another process is runnable
From: Jason Wang @ 2014-09-03 6:58 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Michael S. Tsirkin, Mike Galbraith, davem, netdev, linux-kernel,
Ingo Molnar, Eliezer Tamir
In-Reply-To: <20140902102410.GX27892@worktop.ger.corp.intel.com>
On 09/02/2014 06:24 PM, Peter Zijlstra wrote:
> On Tue, Sep 02, 2014 at 12:03:42PM +0800, Jason Wang wrote:
>> > On 09/01/2014 06:19 PM, Peter Zijlstra wrote:
>>> > > OK I suppose that more or less makes sense, the contextual behaviour is
>>> > > of course tedious in that it makes behaviour less predictable. The
>>> > > 'other' tasks might not want to generate data and you then destroy
>>> > > throughput by not spinning.
>> >
>> > The patch try to make sure:
>> > - the the performance of busy read was not worse than it was disabled in
>> > any cases.
>> > - the performance improvement of a single socket was not achieved by
>> > sacrificing the total performance (all other processes) of the system
>> >
>> > If 'other' tasks are also CPU or I/O intensive jobs, we switch to do
>> > them so the total performance were kept or even increased, and the
>> > performance of current process were guaranteed not worse than when busy
>> > read was disabled (or even better since it may still do busy read
>> > sometimes when it was the only runnable process). If 'other' task are
>> > not intensive, they just do little work and sleep soon, then the busy
>> > read can still work in most of the time during the future reads, we may
>> > still get obvious improvements
> Not entirely true; the select/poll whatever will now block, which means
> we need a wakeup, which increases the latency immensely.
Not sure I get your meaning. This patch does not change the logic or
dynamic of select/poll since sock_poll() always call sk_busy_loop() with
noblock is true. This means sk_busy_loop() will only try ndo_busy_poll()
once whatever the result of other checks. The busy polling was done
through its caller in fact.
>>> > > I'm not entirely sure I see how its all supposed to work though; the
>>> > > various poll functions call sk_busy_poll() and do_select() also loops.
>>> > >
>>> > > The patch only kills the sk_busy_poll() loop, but then do_select() will
>>> > > still loop and not sleep, so how is this helping?
>> >
>> > Yes, the patch only help for processes who did a blocking reads (busy
>> > read). For select(), maybe we can do the same thing but need more test
>> > and thoughts.
> What's the blocking read callgraph, how so we end up in sk_busy_poll() there?
>
> But that's another reason the patch is wrong.
The patch only try to improve the performance of busy read (and test
results shows impressive changes). It does not change anything for busy
poll. Considering there maybe two processes in one cpu, one is doing
busy read and one is doing busy polling. This patch may in fact help the
busy polling performance in this case.
It's good to discuss the ideas of busy poll together, but it was out of
the scope of this patch. We can try to do optimization on top.
^ permalink raw reply
* Re: [PATCH net-next 2/2] net: exit busy loop when another process is runnable
From: Eliezer Tamir @ 2014-09-03 6:49 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jason Wang, Ingo Molnar, Mike Galbraith, davem, netdev,
linux-kernel, Peter Zijlstra, Ingo Molnar
In-Reply-To: <20140902083124.GB10356@redhat.com>
On 02/09/2014 11:31, Michael S. Tsirkin wrote:
> On Tue, Sep 02, 2014 at 09:15:18AM +0300, Eliezer Tamir wrote:
>> On 02/09/2014 06:29, Jason Wang wrote:
>>> On 09/01/2014 02:39 PM, Eliezer Tamir wrote:
>>>> On 29/08/2014 06:08, Jason Wang wrote:
>>>>>> Yes, but rx busy polling only works in process context and does not
>>>>>> disable bh, so it may be not an issue.
>>>> sk_busy_loop() uses rcu_read_lock_bh(), so it does run with bh disabled.
>>>
>>> True, so we need probably also exit the loop when there are pending bhs.
>>
>> I'm not so sure, in the typical busy poll scenario, the incoming
>> traffic is the most time-critical thing in the system.
>> It's so important that you are willing to trade lots of CPU power
>> for better latency. The user has decided that he wants to dedicate
>> this CPU mostly for that. This is not something that plays nice with
>> other apps, but this is what the user wants.
>
> I think most applications wouldn't interpret this flag as "burn up CPU I don't
> care what is the result", what apps want is more of "maximise throughput
> and minimise latency even if throughput/CPU ratio goes down".
> Jason posted benchmarks that show throughput going up because other
> processes get more of a chance to run, so this seems consistent
> with that goal.
Busy polling is not a general purpose feature, it's not something you
can casually turn on and will "just work". Most applications should not
be using busy polling. Currently it is used by multiserver applications
that you spend days tuning to specific platforms.
What the user wants is to lower both avg and maximum latencies, at the
expense of everything else including power efficiency and sometimes
even throughput. The only exception is making the system crash ;)
While letting other things take precedence over busy polling might not
hurt the avg latency much, it will kill your maximum latency.
-Eliezer
^ permalink raw reply
* RE: [PATCH] fec_main iMX6Q DMA-API: device driver tries to free DMA memory it has not allocated
From: fugang.duan @ 2014-09-03 6:36 UTC (permalink / raw)
To: Anand Moon, David S. Miller, Russell King, Frank.Li@freescale.com,
Fabio.Estevam@freescale.com, Uwe Kleine-König
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <1409658022-30166-1-git-send-email-moon.linux@yahoo.com>
From: Anand Moon <moon.linux@yahoo.com> Sent: Tuesday, September 02, 2014 7:40 PM
>To: David S. Miller; Duan Fugang-B38611; Russell King; Li Frank-B20596;
>Estevam Fabio-R49496; Uwe Kleine-König
>Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Anand Moon
>Subject: [PATCH] fec_main iMX6Q DMA-API: device driver tries to free DMA
>memory it has not allocated
>
>Changes fix below warning:
>
>[ 167.331417] ------------[ cut here ]------------ [ 167.331456] WARNING:
>CPU: 0 PID: 1887 at lib/dma-debug.c:1080 check_unmap+0x7e4/0xb90()
>[ 167.331469] fec 2188000.ethernet: DMA-API: device driver tries to free
>DMA memory it has not allocated [device address=0x0000000000000000]
>[size=54 bytes] [ 167.331477] Modules linked in: zram lz4_compress rfcomm
>bnep bluetooth arc4 rt2800usb rt2800lib rt2x00usb rt2x00lib mac80211
>cfg80211 crc_ccitt rfkill rtc_snvs
>[ 167.331560] CPU: 0 PID: 1887 Comm: sshd Tainted: G W 3.17.0-rc3-
>armv7-x1 #1
>[ 167.331592] [<c001990c>] (unwind_backtrace) from [<c00141bc>]
>(show_stack+0x20/0x24) [ 167.331618] [<c00141bc>] (show_stack) from
>[<c0a8601c>] (dump_stack+0x9c/0xbc) [ 167.331647] [<c0a8601c>]
>(dump_stack) from [<c005586c>] (warn_slowpath_common+0x80/0x9c)
>[ 167.331665] [<c005586c>] (warn_slowpath_common) from [<c00558c8>]
>(warn_slowpath_fmt+0x40/0x48) [ 167.331682] [<c00558c8>]
>(warn_slowpath_fmt) from [<c05c377c>] (check_unmap+0x7e4/0xb90)
>[ 167.331699] [<c05c377c>] (check_unmap) from [<c05c3b98>]
>(debug_dma_unmap_page+0x70/0x78) [ 167.331725] [<c05c3b98>]
>(debug_dma_unmap_page) from [<c07678f0>] (fec_enet_rx_napi+0x5dc/0x8bc)
>[ 167.331751] [<c07678f0>] (fec_enet_rx_napi) from [<c0919ce0>]
>(net_rx_action+0x170/0x36c) [ 167.331769] [<c0919ce0>] (net_rx_action)
>from [<c005acbc>] (__do_softirq+0x1a0/0x558) [ 167.331783] [<c005acbc>]
>(__do_softirq) from [<c005b3d0>] (irq_exit+0xd0/0x114) [ 167.331804]
>[<c005b3d0>] (irq_exit) from [<c001064c>] (handle_IRQ+0x54/0xa0)
>[ 167.331820] [<c001064c>] (handle_IRQ) from [<c0008868>]
>(gic_handle_irq+0x3c/0x6c) [ 167.331839] [<c0008868>] (gic_handle_irq)
>from [<c0a8ef84>] (__irq_svc+0x44/0x5c) [ 167.331849] Exception
>stack(0xe41fbc40 to 0xe41fbc88) [ 167.331862] bc40: c00abd3c c0a8e0f0
>00000000 00000000 a0000013 c1aa83c0 c1aa83c0 c1aa83c0 [ 167.331874] bc60:
>00000001 000005b4 00000002 e41fbc9c c117d6e0 e41fbc88 c00abadc c0a8e0f4
>[ 167.331883] bc80: 60000013 ffffffff [ 167.331899] [<c0a8ef84>]
>(__irq_svc) from [<c0a8e0f4>] (_raw_spin_unlock_irqrestore+0x44/0x6c)
>[ 167.331918] [<c0a8e0f4>] (_raw_spin_unlock_irqrestore) from [<c00d8464>]
>(mod_timer+0x13c/0x2c0) [ 167.331938] [<c00d8464>] (mod_timer) from
>[<c08ff824>] (sk_reset_timer+0x28/0x50) [ 167.331957] [<c08ff824>]
>(sk_reset_timer) from [<c0978008>] (tcp_rearm_rto+0xa0/0x104)
>[ 167.331973] [<c0978008>] (tcp_rearm_rto) from [<c097b574>]
>(tcp_event_new_data_sent+0xa0/0xa4)
>[ 167.331989] [<c097b574>] (tcp_event_new_data_sent) from [<c097d518>]
>(tcp_write_xmit+0x1cc/0xd2c) [ 167.332004] [<c097d518>] (tcp_write_xmit)
>from [<c097e450>] (tcp_push_one+0x44/0x50) [ 167.332018] [<c097e450>]
>(tcp_push_one) from [<c096f3b8>] (tcp_sendmsg+0xbf8/0xd68) [ 167.332039]
>[<c096f3b8>] (tcp_sendmsg) from [<c09a17fc>] (inet_sendmsg+0x15c/0x25c)
>[ 167.332064] [<c09a17fc>] (inet_sendmsg) from [<c08f9e74>]
>(sock_aio_write+0xf8/0x118) [ 167.332091] [<c08f9e74>] (sock_aio_write)
>from [<c01e9560>] (do_sync_write+0x90/0xb8) [ 167.332108] [<c01e9560>]
>(do_sync_write) from [<c01ea12c>] (vfs_write+0x158/0x1e0) [ 167.332123]
>[<c01ea12c>] (vfs_write) from [<c01ea7e4>] (SyS_write+0x54/0xb0)
>[ 167.332140] [<c01ea7e4>] (SyS_write) from [<c000fc20>]
>(ret_fast_syscall+0x0/0x48) [ 167.332150] ---[ end trace
>e95c276a4fb5c451 ]---
>
>Signed-off-by: Anand Moon <moon.linux@yahoo.com>
>---
> drivers/net/ethernet/freescale/fec_main.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/net/ethernet/freescale/fec_main.c
>b/drivers/net/ethernet/freescale/fec_main.c
>index 89355a7..b1d9b79 100644
>--- a/drivers/net/ethernet/freescale/fec_main.c
>+++ b/drivers/net/ethernet/freescale/fec_main.c
>@@ -444,8 +444,9 @@ dma_mapping_error:
> bdp = fep->cur_tx;
> for (i = 0; i < frag; i++) {
> bdp = fec_enet_get_nextdesc(bdp, fep);
>- dma_unmap_single(&fep->pdev->dev, bdp->cbd_bufaddr,
>- bdp->cbd_datlen, DMA_TO_DEVICE);
>+ if (bdp->cbd_bufaddr > 0)
>+ dma_unmap_single(&fep->pdev->dev, bdp->cbd_bufaddr,
>+ bdp->cbd_datlen, DMA_TO_DEVICE);
> }
I don't think there need to check cbd_bufaddr ?
> return NETDEV_TX_OK;
> }
>@@ -1106,7 +1107,8 @@ fec_enet_tx(struct net_device *ndev)
>
> skb = fep->tx_skbuff[index];
> fep->tx_skbuff[index] = NULL;
>- if (!IS_TSO_HEADER(fep, bdp->cbd_bufaddr))
>+ if (!IS_TSO_HEADER(fep, bdp->cbd_bufaddr) &&
>+ bdp->cbd_bufaddr > 0)
> dma_unmap_single(&fep->pdev->dev, bdp->cbd_bufaddr,
> bdp->cbd_datlen, DMA_TO_DEVICE);
> bdp->cbd_bufaddr = 0;
>@@ -2111,8 +2113,11 @@ static void fec_enet_free_buffers(struct net_device
>*ndev)
> skb = fep->rx_skbuff[i];
> fep->rx_skbuff[i] = NULL;
> if (skb) {
>- dma_unmap_single(&fep->pdev->dev, bdp->cbd_bufaddr,
>- FEC_ENET_RX_FRSIZE, DMA_FROM_DEVICE);
>+ if (bdp->cbd_bufaddr > 0)
>+ dma_unmap_single(&fep->pdev->dev,
>+ bdp->cbd_bufaddr,
>+ FEC_ENET_RX_FRSIZE,
>+ DMA_FROM_DEVICE);
> dev_kfree_skb(skb);
> }
> bdp = fec_enet_get_nextdesc(bdp, fep);
>--
>1.9.1
Thanks,
Andy
^ permalink raw reply
* Re: [net-next PATCH v2 02/15] net: rcu-ify tcf_proto
From: John Fastabend @ 2014-09-03 6:23 UTC (permalink / raw)
To: David Miller; +Cc: xiyou.wangcong, jhs, eric.dumazet, netdev, paulmck, brouer
In-Reply-To: <20140902.135218.623659943093020151.davem@davemloft.net>
On 09/02/2014 01:52 PM, David Miller wrote:
> From: John Fastabend <john.fastabend@gmail.com>
> Date: Mon, 01 Sep 2014 18:39:13 -0700
>
>> On 08/24/2014 10:31 PM, David Miller wrote:
>>> From: John Fastabend <john.fastabend@gmail.com>
>>> Date: Sun, 24 Aug 2014 17:48:31 -0700
>>>
>>>> @@ -722,8 +724,9 @@ static void sfq_free(void *addr)
>>>> static void sfq_destroy(struct Qdisc *sch)
>>>> {
>>>> struct sfq_sched_data *q = qdisc_priv(sch);
>>>> + struct tcf_proto *fl = rtnl_dereference(q->filter_list);
>>>>
>>>> - tcf_destroy_chain(&q->filter_list);
>>>> + tcf_destroy_chain(&fl);
>>
>> Sorry for the delayed reply...
>>
>>>
>>> This will cause tcf_destroy_chain() to set the local variable
>>> 'fl' to NULL rather than q->filter_list.
>>>
>>> I don't see how this can be correct at all.
>>
>> Right now (without these patches) nothing sets q->filter_list
>> to NULL and we only call this when the qdisc is being destroyed.
>
> Yes, it does set it to NULL, by virtue of how the loop iterates.
>
yes of course its obvious now that you spell it out for me.
> It iterates by setting the "*fl" to tp->next until that evaluates to
> NULL. Thereby setting *fl to NULL.
>
> So the old code would set ->filter_list to NULL, your code will not.
>
right I will fix this in the morning along with the sparse
warnings related to the tcf_find signatures.
Thanks
John
--
John Fastabend Intel Corporation
^ permalink raw reply
* Re: [PATCH net-next 2/2] net: exit busy loop when another process is runnable
From: Eliezer Tamir @ 2014-09-03 6:21 UTC (permalink / raw)
To: Jason Wang, Eric Dumazet
Cc: Ingo Molnar, Mike Galbraith, davem, netdev, linux-kernel, mst,
Peter Zijlstra, Ingo Molnar jacob.e.keller@intel.com
In-Reply-To: <54056459.4060100@redhat.com>
On 02/09/2014 09:31, Jason Wang wrote:
> On 09/02/2014 02:03 PM, Eliezer Tamir wrote:
>> On 02/09/2014 06:35, Jason Wang wrote:
>>> Not sure whether this method will scale considering thousands of sockets
>>> and processes.
>> There may be millions of sockets, but in most cases only a handful of
>> device queues per CPU to busy poll on. I have tested the epoll rfc
>> code with hundreds of thousands of sockets and one or two device
>> queues and is scales pretty well.
>>
>> The part I don't like in that code is the cumbersome mechanism I used
>> to track the socket -> queue relationship. I think that if I had more
>> time to work on it, I would instead look into extending the epoll
>> interface so that libevent can tell the kernel what it wants, instead
>> of having the busypoll code try and learn it.
>
> I'd like to have a look at this rfc. Could you please give me a pointer?
> I've done a quick search on kernel mailing list but didn't find it.
https://lkml.org/lkml/2013/8/21/192
Cheers,
Eliezer
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox