* Re: [PATCH 0/4] make POSIX timers optional with some Kconfig help
From: Josh Triplett @ 2016-10-20 18:51 UTC (permalink / raw)
To: Nicolas Pitre
Cc: John Stultz, Richard Cochran, Yann E MORIN, Thomas Gleixner,
netdev, linux-kbuild, linux-kernel
In-Reply-To: <1476920573-14384-1-git-send-email-nicolas.pitre@linaro.org>
On Wed, Oct 19, 2016 at 07:42:49PM -0400, Nicolas Pitre wrote:
> Many embedded systems don't need the full POSIX timer support.
> Configuring them out provides a nice kernel image size reduction.
>
> When POSIX timers are configured out, the PTP clock subsystem should be
> left out as well. However a bunch of ethernet drivers currently *select*
> the later in their Kconfig entries. Therefore some more work was needed
> to break that hard dependency from those drivers without preventing their
> usage altogether.
>
> Therefore this series also includes kconfig changes to implement a new
> keyword to express some reverse dependencies like "select" does, named
> "imply", and still allowing for the target config symbol to be disabled
> if the user or a direct dependency says so.
>
> How to deal with the dependencies across three subsystems for potential
> upstream merging needs to be figured out.
This looks good to me, and I like the new "imply" approach.
I'd still like to see a more general solution for reporting the use of
compiled-out syscalls, but I don't think that needs to block this patch
series.
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
^ permalink raw reply
* Re: [PATCH net-next v2 0/9] net: use core MTU range checking everywhere
From: David Miller @ 2016-10-20 18:53 UTC (permalink / raw)
To: jarod; +Cc: linux-kernel, netdev
In-Reply-To: <20161020175524.6184-1-jarod@redhat.com>
From: Jarod Wilson <jarod@redhat.com>
Date: Thu, 20 Oct 2016 13:55:15 -0400
> This stack of patches should get absolutely everything in the kernel
> converted from doing their own MTU range checking to the core MTU range
> checking. This second spin includes alterations to hopefully fix all
> concerns raised with the first, as well as including some additional
> changes to drivers and infrastructure where I completely missed necessary
> updates.
>
> These have all been built through the 0-day build infrastructure via the
> (rebasing) master branch at https://github.com/jarodwilson/linux-muck, which
> at the time of the most recent compile across 147 configs, was based on
> net-next at commit 7b1536ef0aa0.
Series applied, hopefully this gets most of the fallout.
Thanks Jarod.
^ permalink raw reply
* Re: [Patch net] net: saving irq context for peernet2id()
From: Paul Moore @ 2016-10-20 19:07 UTC (permalink / raw)
To: Cong Wang
Cc: Stephen Smalley, Linux Kernel Network Developers, Elad Raz,
Richard Guy Briggs
In-Reply-To: <CAM_iQpWD3WiX1ZX+3LJ8vbJc42K1jZcZ4-f6MO5kzZY-+Szkqw@mail.gmail.com>
On Thu, Oct 20, 2016 at 2:29 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Thu, Oct 20, 2016 at 7:58 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 10/20/2016 02:52 AM, Cong Wang wrote:
>>> A kernel warning inside __local_bh_enable_ip() was reported by people
>>> running SELinux, this is caused due to some SELinux functions
>>> (indirectly) call peernet2id() with IRQ disabled in process context,
>>> when we re-enable BH with IRQ disabled kernel complains. Shut up this
>>> warning by saving IRQ context in peernet2id(), BH is still implicitly
>>> disabled.
>>
>> Not sure this suffices; kill_fasync() -> send_sigio() ->
>> send_sigio_to_task() -> sigio_perm() -> security_file_send_sigiotask()
>> -> selinux_file_send_sigiotask() -> ... -> audit_log() -> ... ->
>> peernet2id()
>
> Oh, this is a new one. kill_fasync() is called in IRQ handler, so we actually
> do multicast in IRQ context.... It makes no sense, netlink multicast could
> be very expensive if we have many listeners.
I'm sure there are a few others I don't know about, but I believe the
only commonly used audit multicast listener is systemd.
> I am Cc'ing Richard who added that multicast in audit_log_end(). It seems
> not easy to just move the multicast to a workqueue, since the skb is copied
> from audit_buffer which is freed immediately after that, probably need another
> queue like audit_skb_queue.
This approach would double the queue size which is something I want to
avoid. I would suggest sticking with a single queue and dealing with
the netlink message link fixup and multicast send in the existing
netlink unicast thread; basically we would just be moving the
multicast code from audit_log_end() into kauditd_thread(). This is
the same approach I mentioned earlier off-list.
However, that isn't something I want to mess with as a regression fix,
mostly because I really want to see this regression gone by -rc2 as it
is making SELinux testing a real pain. If the patch posted at the top
of this thread isn't a suitable fix, we really should revert the
original patch.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH 1/4] kconfig: introduce the "imply" keyword
From: Edward Cree @ 2016-10-20 19:09 UTC (permalink / raw)
To: Nicolas Pitre
Cc: John Stultz, Richard Cochran, Yann E MORIN, Thomas Gleixner,
Josh Triplett, netdev, linux-kbuild, linux-kernel
In-Reply-To: <alpine.LFD.2.20.1610201349070.25105@knanqh.ubzr>
On 20/10/16 19:29, Nicolas Pitre wrote:
> On Thu, 20 Oct 2016, Edward Cree wrote:
>> But the desire is a property of the user, not of the driver. If you're
>> willing to add CONFIG_FOO_BAZ to every combination of (driver, subsystem)
>> then "imply" becomes unnecessary, doesn't it?
> Absolutely. And if that's something that inspires you please be my
> guest. So far, though, this apparently didn't inspire the majority of
> driver authors who preferred to have a smaller set of config options and
> forcefully pull in the BAZ features with a "select". But "select" comes
> with its set of evils which "imply" is meant to overcome.
It really doesn't inspire me either ;)
I was using it as a way to set up the converse, rather than as any kind of
serious suggestion.
And I agree that "imply", as it stands, is an improvement over "select" for
these kinds of cases. I just think it could be further improved.
>> Conversely, if you *don't*
>> want to have to do that, then "imply" needs to only ever deal in defaults,
>> not in limitations.
> As I explained, It still has to prevent BAZ=m if FOO moves from m to y
> otherwise this would effectively have the same result as BAZ=n in
> practice and that is not what people expect if BAZ actually isn't n in
> your .config file. That's why "select" also has that particular
> semantic.
If FOO "moves from" m to y (by which I'm assuming you mean a call to
sym_set_tristate_value()), then by all means set BAZ=y. But the user
should then still be allowed to move BAZ from y to m without having to
change FOO (though hopefully they will get warned about it somehow).
> Here "imply" is meant to be a weaker form of "select". If you prefer
> not to have that limitation imposed by either "select" and "imply" then
> simply don't use them at all. Nothing forces you to use any of them if
> your code can cope with any config combination.
I'm interpreting "imply" as being more a way of saying "if you want FOO you
probably want BAZ as well". But maybe that should be yet another new
keyword if it's so different from what you want "imply" to be. "suggests",
perhaps.
>> Right, so those drivers can use PTP if they're y and PTP is m, as long
>> as the PTP module is loaded when they probe.
> Not at the moment. There is no way for PTP to dynamically signal to
> interested drivers its presence at run time. And drivers, when
> built-in, typically probe their hardware during the boot process even
> before you have the chance to load any module. If that ever changes,
> then the imply or select statement could simply be dropped.
At least for PCIe devices, the driver can be un- and re-bound (despite
being built-in) through sysfs. So you (already) can re-probe them after
loading PTP. So driver=y && PTP=m is valid, today.
>> I think that Josh's suggestion (have the UI warn you if you set BAZ to m
>> while FOO=y) is the right approach, but I also think it should be done
>> now rather than at some unspecified future time.
> Please advocate this with kconfig UI authors. My recursion stack is
> already quite deep.
If I'm reading your patch correctly, your symbol.c additions enforce this
restriction, and AFAIK the UI can't override that by saying "Yeah I warned
the user and he said it was fine".
The kconfig UI API would need to change; sym_set_tristate_value() could
grow an 'override-imply' flag, for instance.
But I'm not married to the idea; I don't have a real-world use-case this
interferes with, so if you still think I'm wrong, feel free to ignore me :)
-Ed
^ permalink raw reply
* Re: [RFC PATCH net-next] bpf: fix potential percpu map overcopy to user.
From: William Tu @ 2016-10-20 19:21 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: Alexei Starovoitov, Linux Kernel Network Developers
In-Reply-To: <58074581.1000601@iogearbox.net>
> Documentation/cputopology.txt +106 says /sys/devices/system/cpu/possible
> outputs cpu_possible_mask. That is the same as in num_possible_cpus(), so
> first step would be to fix the buggy example code, imho.
>
> What perhaps could be done in a second step to reduce overhead is an option
> for bpf(2) to pass in a cpu mask similarly as for sched_{get,set}affinity()
> syscalls, where user space can construct a mask via CPU_SET(3). For the
> syscall time, kernel would lock hot plugging via get_online_cpus() and
> put_online_cpus(), it would check whether passed CPUs are online to query
> and if so then it would copy the values into the user provided buffer. I'd
> think this might be useful in a number of ways anyway.
>
I like this idea. So in this case, the only the data at the cpu
specified by user in the CPU_SET is copied to userspace, potentially
have better performance than always copying the data *
num_possible_cpus() bytes.
Regards,
William
^ permalink raw reply
* Re: [Patch net] net: saving irq context for peernet2id()
From: Cong Wang @ 2016-10-20 19:04 UTC (permalink / raw)
To: Stephen Smalley
Cc: Linux Kernel Network Developers, Elad Raz, Paul Moore,
Richard Guy Briggs
In-Reply-To: <CAM_iQpWD3WiX1ZX+3LJ8vbJc42K1jZcZ4-f6MO5kzZY-+Szkqw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1377 bytes --]
On Thu, Oct 20, 2016 at 11:29 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Thu, Oct 20, 2016 at 7:58 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 10/20/2016 02:52 AM, Cong Wang wrote:
>>> A kernel warning inside __local_bh_enable_ip() was reported by people
>>> running SELinux, this is caused due to some SELinux functions
>>> (indirectly) call peernet2id() with IRQ disabled in process context,
>>> when we re-enable BH with IRQ disabled kernel complains. Shut up this
>>> warning by saving IRQ context in peernet2id(), BH is still implicitly
>>> disabled.
>>
>> Not sure this suffices; kill_fasync() -> send_sigio() ->
>> send_sigio_to_task() -> sigio_perm() -> security_file_send_sigiotask()
>> -> selinux_file_send_sigiotask() -> ... -> audit_log() -> ... ->
>> peernet2id()
>
> Oh, this is a new one. kill_fasync() is called in IRQ handler, so we actually
> do multicast in IRQ context.... It makes no sense, netlink multicast could
> be very expensive if we have many listeners.
>
> I am Cc'ing Richard who added that multicast in audit_log_end(). It seems
> not easy to just move the multicast to a workqueue, since the skb is copied
> from audit_buffer which is freed immediately after that, probably need another
> queue like audit_skb_queue.
Please let me know if the attached patch makes any sense to you, before
I give it a serious test.
Thanks!
[-- Attachment #2: audit.diff --]
[-- Type: text/plain, Size: 1828 bytes --]
diff --git a/kernel/audit.c b/kernel/audit.c
index f1ca116..cb2b31b 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -139,6 +139,7 @@ static int audit_freelist_count;
static LIST_HEAD(audit_freelist);
static struct sk_buff_head audit_skb_queue;
+static struct sk_buff_head audit_skb_multicast_queue;
/* queue of skbs to send to auditd when/if it comes back */
static struct sk_buff_head audit_skb_hold_queue;
static struct task_struct *kauditd_task;
@@ -468,7 +469,8 @@ static void kauditd_send_multicast_skb(struct sk_buff *skb, gfp_t gfp_mask)
if (!copy)
return;
- nlmsg_multicast(sock, copy, 0, AUDIT_NLGRP_READLOG, gfp_mask);
+ skb_queue_tail(&audit_skb_multicast_queue, copy);
+ wake_up_interruptible(&kauditd_wait);
}
/*
@@ -509,6 +511,25 @@ static void flush_hold_queue(void)
consume_skb(skb);
}
+static void flush_multicast_queue(void)
+{
+ struct audit_net *aunet = net_generic(&init_net, audit_net_id);
+ struct sock *sock = aunet->nlsk;
+ struct sk_buff *skb;
+
+ if (!netlink_has_listeners(sock, AUDIT_NLGRP_READLOG))
+ return;
+
+ skb = skb_dequeue(&audit_skb_multicast_queue);
+ if (likely(!skb))
+ return;
+
+ while (skb) {
+ nlmsg_multicast(sock, skb, 0, AUDIT_NLGRP_READLOG, GFP_KERNEL);
+ skb = skb_dequeue(&audit_skb_multicast_queue);
+ }
+}
+
static int kauditd_thread(void *dummy)
{
set_freezable();
@@ -517,6 +538,8 @@ static int kauditd_thread(void *dummy)
flush_hold_queue();
+ flush_multicast_queue();
+
skb = skb_dequeue(&audit_skb_queue);
if (skb) {
@@ -530,7 +553,8 @@ static int kauditd_thread(void *dummy)
continue;
}
- wait_event_freezable(kauditd_wait, skb_queue_len(&audit_skb_queue));
+ wait_event_freezable(kauditd_wait, skb_queue_len(&audit_skb_queue)
+ || skb_queue_len(&audit_skb_multicast_queue));
}
return 0;
}
^ permalink raw reply related
* Re: [PATCH 00/10] mm: adjust get_user_pages* functions to explicitly pass FOLL_* flags
From: Michal Hocko @ 2016-10-20 19:26 UTC (permalink / raw)
To: Dave Hansen
Cc: Lorenzo Stoakes, linux-mm, Linus Torvalds, Jan Kara, Hugh Dickins,
Rik van Riel, Mel Gorman, Andrew Morton, adi-buildroot-devel,
ceph-devel, dri-devel, intel-gfx, kvm, linux-alpha,
linux-arm-kernel, linux-cris-kernel, linux-fbdev, linux-fsdevel,
linux-ia64, linux-kernel, linux-media, linux-mips, linux-rdma,
linux-s390, linux-samsung-soc
In-Reply-To: <5807AC2B.4090208@linux.intel.com>
On Wed 19-10-16 10:23:55, Dave Hansen wrote:
> On 10/19/2016 10:01 AM, Michal Hocko wrote:
> > The question I had earlier was whether this has to be an explicit FOLL
> > flag used by g-u-p users or we can just use it internally when mm !=
> > current->mm
>
> The reason I chose not to do that was that deferred work gets run under
> a basically random 'current'. If we just use 'mm != current->mm', then
> the deferred work will sometimes have pkeys enforced and sometimes not,
> basically randomly.
OK, I see (async_pf_execute and ksm ). It makes more sense to me. Thanks
for the clarification.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH net] ipv4: disable BH in set_ping_group_range()
From: Cong Wang @ 2016-10-20 19:32 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Eric Salo
In-Reply-To: <1476984408.7065.21.camel@edumazet-glaptop3.roam.corp.google.com>
On Thu, Oct 20, 2016 at 10:26 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> In commit 4ee3bd4a8c746 ("ipv4: disable BH when changing ip local port
> range") Cong added BH protection in set_local_port_range() but missed
> that same fix was needed in set_ping_group_range()
Don't know why ping_group_range shares the same lock with local_port_range...
Perhaps just for saving a few bytes, but that is why I missed this place.
^ permalink raw reply
* Re: [RFC PATCH net-next] bpf: fix potential percpu map overcopy to user.
From: Daniel Borkmann @ 2016-10-20 19:39 UTC (permalink / raw)
To: William Tu, Alexei Starovoitov; +Cc: Linux Kernel Network Developers
In-Reply-To: <CALDO+SZbWbMPFsj10hR3XEB1696s0ENRoBttrT5T3O=DQ445XQ@mail.gmail.com>
On 10/20/2016 08:41 PM, William Tu wrote:
> On Thu, Oct 20, 2016 at 9:58 AM, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>> On Thu, Oct 20, 2016 at 06:04:38PM +0200, Daniel Borkmann wrote:
>>>
>>> diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
>>> index ee384f0..d4832e8 100644
>>> --- a/tools/testing/selftests/bpf/test_maps.c
>>> +++ b/tools/testing/selftests/bpf/test_maps.c
>>> @@ -25,6 +25,33 @@
>>>
>>> static int map_flags;
>>>
>>> +static unsigned int num_possible_cpus(void)
>>> +{
>>> + static const char *fcpu = "/sys/devices/system/cpu/possible";
>>> + unsigned int val, possible_cpus = 0;
>>> + char buff[128];
>>> + FILE *fp;
>>> +
>>> + fp = fopen(fcpu, "r");
>>> + if (!fp) {
>>> + printf("Failed to open %s: '%s'!\n", fcpu, strerror(errno));
>>> + exit(1);
>>> + }
>>> +
>>> + while (fgets(buff, sizeof(buff), fp)) {
>>> + if (sscanf(buff, "%*u-%u", &val) == 1)
>>> + possible_cpus = val;
>>> + }
>>
>> looks great to me.
>> Could you move it into bpf_sys.h or somehow make it common in libbpf
>> and reuse it in samples/bpf/ ?
>> Since quite a few samples need this fix as well.
Ahh, true.
>> Thanks!
>
> Looks good to me. I tested it and it works fine.
Okay, thanks. I'll fix that up, mid-term we should try and move most of
that over to kernel selftests/bpf, and reuse tools/lib/bpf/.
> Thanks!
> William
^ permalink raw reply
* Re: [PATCH net] ipv4: disable BH in set_ping_group_range()
From: Cong Wang @ 2016-10-20 19:40 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Eric Salo
In-Reply-To: <CAM_iQpVFQD0Oqf8YO9HybDpPMhzMMK5m_wNyLwTfVpEwpAVBNw@mail.gmail.com>
On Thu, Oct 20, 2016 at 12:32 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Thu, Oct 20, 2016 at 10:26 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> From: Eric Dumazet <edumazet@google.com>
>>
>> In commit 4ee3bd4a8c746 ("ipv4: disable BH when changing ip local port
>> range") Cong added BH protection in set_local_port_range() but missed
>> that same fix was needed in set_ping_group_range()
>
> Don't know why ping_group_range shares the same lock with local_port_range...
> Perhaps just for saving a few bytes, but that is why I missed this place.
Hold on... We clearly have typos there... Your fix is not correct.
^ permalink raw reply
* Re: [PATCH net] ipv4: disable BH in set_ping_group_range()
From: Cong Wang @ 2016-10-20 19:44 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Eric Salo
In-Reply-To: <CAM_iQpVBHa2ujKN1hsU0LT+g899+WEy9a1oBgDj1FmpifPmz6g@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 872 bytes --]
On Thu, Oct 20, 2016 at 12:40 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Thu, Oct 20, 2016 at 12:32 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Thu, Oct 20, 2016 at 10:26 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>> From: Eric Dumazet <edumazet@google.com>
>>>
>>> In commit 4ee3bd4a8c746 ("ipv4: disable BH when changing ip local port
>>> range") Cong added BH protection in set_local_port_range() but missed
>>> that same fix was needed in set_ping_group_range()
>>
>> Don't know why ping_group_range shares the same lock with local_port_range...
>> Perhaps just for saving a few bytes, but that is why I missed this place.
>
> Hold on... We clearly have typos there... Your fix is not correct.
We need the attached patch, your patch should be reverted, because
unlike local_port_range we never read it in BH context, no need to bother _bh.
[-- Attachment #2: ping.diff --]
[-- Type: text/plain, Size: 1211 bytes --]
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 1cb67de..80bc36b 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -96,11 +96,11 @@ static void inet_get_ping_group_range_table(struct ctl_table *table, kgid_t *low
container_of(table->data, struct net, ipv4.ping_group_range.range);
unsigned int seq;
do {
- seq = read_seqbegin(&net->ipv4.ip_local_ports.lock);
+ seq = read_seqbegin(&net->ipv4.ping_group_range.lock);
*low = data[0];
*high = data[1];
- } while (read_seqretry(&net->ipv4.ip_local_ports.lock, seq));
+ } while (read_seqretry(&net->ipv4.ping_group_range.lock, seq));
}
/* Update system visible IP port range */
@@ -109,10 +109,10 @@ static void set_ping_group_range(struct ctl_table *table, kgid_t low, kgid_t hig
kgid_t *data = table->data;
struct net *net =
container_of(table->data, struct net, ipv4.ping_group_range.range);
- write_seqlock(&net->ipv4.ip_local_ports.lock);
+ write_seqlock(&net->ipv4.ping_group_range.lock);
data[0] = low;
data[1] = high;
- write_sequnlock(&net->ipv4.ip_local_ports.lock);
+ write_sequnlock(&net->ipv4.ping_group_range.lock);
}
/* Validate changes from /proc interface. */
^ permalink raw reply related
* [PATCH] net: l2tp_eth: fix max_mtu
From: Asbjoern Sloth Toennesen @ 2016-10-20 20:08 UTC (permalink / raw)
To: David S . Miller; +Cc: linux-kernel, netdev, Jarod Wilson
Fixes: 61e84623ace3 ("net: centralize net_device min/max MTU checking")
CC: netdev@vger.kernel.org
CC: Jarod Wilson <jarod@redhat.com>
Signed-off-by: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
---
net/l2tp/l2tp_eth.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index 965f7e3..ba82dcc 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -259,6 +259,7 @@ static int l2tp_eth_create(struct net *net, u32 tunnel_id, u32 session_id, u32 p
session->mtu = dev->mtu - session->hdr_len;
dev->mtu = session->mtu;
dev->needed_headroom += session->hdr_len;
+ dev->max_mtu = ETH_MAX_MTU - dev->needed_headroom;
priv = netdev_priv(dev);
priv->dev = dev;
--
2.9.3
^ permalink raw reply related
* Re: [PATCH] davinci_emac: fix setting the mac from DT
From: Jeroen Hofstee @ 2016-10-20 13:35 UTC (permalink / raw)
To: Tony Lindgren
Cc: netdev, David S . Miller, Anant Gole, Chaithrika U S, linux-omap
In-Reply-To: <20161020124149.v47amnfbisndai4q@atomide.com>
Hi,
On 20-10-16 14:41, Tony Lindgren wrote:
> * Jeroen Hofstee <jhofstee@victronenergy.com> [161019 12:39]:
>> commit 9120bd6e9f77 ("net: davinci_emac: Get device dm816x MAC address
>> using the cpsw code") sets the mac address to the one stored in the chip
>> unconditionally, overwritten the one already set from the device tree.
>> This patch makes sure the mac from DT is preserved.
>>
>> On a am3517 this address is incorrectly read as all zeros, making it
>> impossible to set a valid mac address without this patch.
> OK, at least I don't have better ideas for fixing this:
more details about the am3517 specific issue can be found
here http://marc.info/?l=linux-omap&m=147678889732646&w=2
Regards,
Jeroen
^ permalink raw reply
* Re: [PATCH 1/4] kconfig: introduce the "imply" keyword
From: Nicolas Pitre @ 2016-10-20 20:10 UTC (permalink / raw)
To: Edward Cree
Cc: John Stultz, Richard Cochran, Yann E MORIN, Thomas Gleixner,
Josh Triplett, netdev, linux-kbuild, linux-kernel
In-Reply-To: <3c91236f-6795-fdcc-c8a2-6e7b2fd7da66@solarflare.com>
On Thu, 20 Oct 2016, Edward Cree wrote:
> On 20/10/16 19:29, Nicolas Pitre wrote:
> > On Thu, 20 Oct 2016, Edward Cree wrote:
> >> But the desire is a property of the user, not of the driver. If you're
> >> willing to add CONFIG_FOO_BAZ to every combination of (driver, subsystem)
> >> then "imply" becomes unnecessary, doesn't it?
> > Absolutely. And if that's something that inspires you please be my
> > guest. So far, though, this apparently didn't inspire the majority of
> > driver authors who preferred to have a smaller set of config options and
> > forcefully pull in the BAZ features with a "select". But "select" comes
> > with its set of evils which "imply" is meant to overcome.
> It really doesn't inspire me either ;)
> I was using it as a way to set up the converse, rather than as any kind of
> serious suggestion.
> And I agree that "imply", as it stands, is an improvement over "select" for
> these kinds of cases. I just think it could be further improved.
> >> Conversely, if you *don't*
> >> want to have to do that, then "imply" needs to only ever deal in defaults,
> >> not in limitations.
> > As I explained, It still has to prevent BAZ=m if FOO moves from m to y
> > otherwise this would effectively have the same result as BAZ=n in
> > practice and that is not what people expect if BAZ actually isn't n in
> > your .config file. That's why "select" also has that particular
> > semantic.
> If FOO "moves from" m to y (by which I'm assuming you mean a call to
> sym_set_tristate_value()), then by all means set BAZ=y. But the user
> should then still be allowed to move BAZ from y to m without having to
> change FOO (though hopefully they will get warned about it somehow).
The case I'm most worried about is:
- open .config in $EDITOR
- change "CONFIG_FOO=m" to "CONFIG_FOO=y"
- save and exit
- make
The warning is most likely to be missed in that case, and if .config
doesn't list CONFIG_BAZ as unset then this is likely to confuse people.
> > Here "imply" is meant to be a weaker form of "select". If you prefer
> > not to have that limitation imposed by either "select" and "imply" then
> > simply don't use them at all. Nothing forces you to use any of them if
> > your code can cope with any config combination.
> I'm interpreting "imply" as being more a way of saying "if you want FOO you
> probably want BAZ as well". But maybe that should be yet another new
> keyword if it's so different from what you want "imply" to be. "suggests",
> perhaps.
Indeed. That's exactly the keyword that came to my mind after I sent my
previous reply.
> >> Right, so those drivers can use PTP if they're y and PTP is m, as long
> >> as the PTP module is loaded when they probe.
> > Not at the moment. There is no way for PTP to dynamically signal to
> > interested drivers its presence at run time. And drivers, when
> > built-in, typically probe their hardware during the boot process even
> > before you have the chance to load any module. If that ever changes,
> > then the imply or select statement could simply be dropped.
> At least for PCIe devices, the driver can be un- and re-bound (despite
> being built-in) through sysfs. So you (already) can re-probe them after
> loading PTP. So driver=y && PTP=m is valid, today.
I agree with the principle, but the implementation just doesn't allow it
at the moment. In a simplified form, what's there now in ptp.h is:
#if IS_REACHABLE(CONFIG_PTP)
extern struct ptp_clock *ptp_clock_register(...);
#else
static inline struct ptp_clock *ptp_clock_register(...) { return NULL; }
#endif
i.e. drivers may cope at runtime with PTP being absent, but there is no
support for PTP to be loaded after drivers referring to it. Hence the
use of "select" that I simply converted to "imply".
> >> I think that Josh's suggestion (have the UI warn you if you set BAZ to m
> >> while FOO=y) is the right approach, but I also think it should be done
> >> now rather than at some unspecified future time.
> > Please advocate this with kconfig UI authors. My recursion stack is
> > already quite deep.
> If I'm reading your patch correctly, your symbol.c additions enforce this
> restriction, and AFAIK the UI can't override that by saying "Yeah I warned
> the user and he said it was fine".
> The kconfig UI API would need to change; sym_set_tristate_value() could
> grow an 'override-imply' flag, for instance.
The UI may tell the user about the restriction and suggest a way to
overcome it by also changing the "impliers" to m.
Otherwise I much prefer to have a kconfig keyword that expresses the
fact that it is actually OK to override it, like this "suggests" idea
could do.
Sidenote: the kconfig language isn't coherent wrt english grammar as
there is "depends on" but "select" rather than "selects". I interpreted
"select" as using the imperative mood, hence the addition of "imply"
rather than "implies".
Nicolas
^ permalink raw reply
* RE: [PATCH net-next v2 6/9] net: use core MTU range checking in virt drivers
From: Kershner, David A @ 2016-10-20 20:12 UTC (permalink / raw)
To: Haiyang Zhang, Jarod Wilson, linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org, virtualization@lists.linux-foundation.org,
KY Srinivasan, Michael S. Tsirkin, Shrikrishna Khare,
VMware, Inc., Wei Liu, Paul Durrant
In-Reply-To: <BLUPR03MB141270CC6037546F918409AECAD50@BLUPR03MB1412.namprd03.prod.outlook.com>
> -----Original Message-----
> From: Haiyang Zhang [mailto:haiyangz@microsoft.com]
> Sent: Thursday, October 20, 2016 2:05 PM
> To: Jarod Wilson <jarod@redhat.com>; linux-kernel@vger.kernel.org
> Cc: netdev@vger.kernel.org; virtualization@lists.linux-foundation.org; KY
> Srinivasan <kys@microsoft.com>; Michael S. Tsirkin <mst@redhat.com>;
> Shrikrishna Khare <skhare@vmware.com>; VMware, Inc. <pv-
> drivers@vmware.com>; Wei Liu <wei.liu2@citrix.com>; Paul Durrant
> <paul.durrant@citrix.com>; Kershner, David A
> <David.Kershner@unisys.com>
> Subject: RE: [PATCH net-next v2 6/9] net: use core MTU range checking in virt
> drivers
>
>
>
> > -----Original Message-----
> > From: Jarod Wilson [mailto:jarod@redhat.com]
> > Sent: Thursday, October 20, 2016 1:55 PM
> > To: linux-kernel@vger.kernel.org
> > Cc: Jarod Wilson <jarod@redhat.com>; netdev@vger.kernel.org;
> > virtualization@lists.linux-foundation.org; KY Srinivasan
> > <kys@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>;
> Michael S.
> > Tsirkin <mst@redhat.com>; Shrikrishna Khare <skhare@vmware.com>;
> VMware,
> > Inc. <pv-drivers@vmware.com>; Wei Liu <wei.liu2@citrix.com>; Paul
> > Durrant <paul.durrant@citrix.com>; David Kershner
> > <david.kershner@unisys.com>
> > Subject: [PATCH net-next v2 6/9] net: use core MTU range checking in
> > virt drivers
> >
> > hyperv_net:
> > - set min/max_mtu, per Haiyang, after rndis_filter_device_add
> >
> > virtio_net:
> > - set min/max_mtu
> > - remove virtnet_change_mtu
> >
> > vmxnet3:
> > - set min/max_mtu
> >
> > xen-netback:
> > - min_mtu = 0, max_mtu = 65517
> >
> > xen-netfront:
> > - min_mtu = 0, max_mtu = 65535
> >
> > unisys/visor:
> > - clean up defines a little to not clash with network core or add
> > redundat definitions
> >
> > CC: netdev@vger.kernel.org
> > CC: virtualization@lists.linux-foundation.org
> > CC: "K. Y. Srinivasan" <kys@microsoft.com>
> > CC: Haiyang Zhang <haiyangz@microsoft.com>
> > CC: "Michael S. Tsirkin" <mst@redhat.com>
> > CC: Shrikrishna Khare <skhare@vmware.com>
> > CC: "VMware, Inc." <pv-drivers@vmware.com>
> > CC: Wei Liu <wei.liu2@citrix.com>
> > CC: Paul Durrant <paul.durrant@citrix.com>
> > CC: David Kershner <david.kershner@unisys.com>
> > Signed-off-by: Jarod Wilson <jarod@redhat.com>
> > ---
>
> The hv_netvsc changes look fine. Thanks.
>
> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
>
The visornic changes look good.
Reviewed-by: David Kershner <david.kershner@unisys.com>
^ permalink raw reply
* RE: [PATCH net-next V2 1/9] liquidio CN23XX: HW config for VF support
From: Vatsavayi, Raghu @ 2016-10-20 20:01 UTC (permalink / raw)
To: David Miller
Cc: netdev@vger.kernel.org, Chickles, Derek, Burla, Satananda,
Manlunas, Felix
In-Reply-To: <20161020.141304.286994718471308700.davem@davemloft.net>
> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Thursday, October 20, 2016 11:13 AM
> To: Vatsavayi, Raghu
> Cc: netdev@vger.kernel.org; Vatsavayi, Raghu; Chickles, Derek; Burla,
> Satananda; Manlunas, Felix
> Subject: Re: [PATCH net-next V2 1/9] liquidio CN23XX: HW config for VF
> support
>
> From: Raghu Vatsavayi <rvatsavayi@caviumnetworks.com>
> Date: Wed, 19 Oct 2016 22:40:38 -0700
>
> > +/* Default behaviour of Liquidio is to provide one queue per VF. But
> > +Liquidio
> > + * can also provide multiple queues to each VF. If user wants to
> > +change the
> > + * default behaviour HW should be provided configuration info at init
> > +time,
> > + * based on which it will create control queues for communicating with
> FW.
> > + */
> > +static u32 max_vfs[2] = { 0, 0 };
> > +module_param_array(max_vfs, int, NULL, 0444);
> > +MODULE_PARM_DESC(max_vfs, "Assign two comma-separated unsigned
> > +integers that specify max number of VFs for PF0 (left of the comma)
> > +and PF1 (right of the comma); for 23xx only. By default HW will
> > +configure as many VFs as queues after allocating PF queues.To
> > +increase queues for VF use this parameter. Use sysfs to create these
> > +VFs.");
> > +
> > +static unsigned int num_queues_per_pf[2] = { 0, 0 };
> > +module_param_array(num_queues_per_pf, uint, NULL, 0444);
> > +MODULE_PARM_DESC(num_queues_per_pf, "two comma-separated
> unsigned
> > +integers that specify number of queues per PF0 (left of the comma)
> > +and PF1 (right of the comma); for 23xx only");
> > +
> > static int ptp_enable = 1;
>
> We cannot continue to allow drivers to add custom module parameters to
> control this. It is the worst user experience possible.
>
> We need a tree-wide generic, consistent, manner in which to configure and
> control this kind of thing.
Sure Dave, I will remove max_vfs module parameter and will use tree wide generic
sysfs interface to enable VFs. Also if user wants to have multiple queues then because
of the way Liquidio HW works we need num_queues_per_pf and num_queues_per_vf
module parameters at HW/module init time. This is required only in case of non-default
case of multi-queues per VF because HW has to carve these queues before FW can start
communicating with PF/VF host drivers, so we may include these two.
I will soon forward you the patches with the changes that you recommended.
Thanks Much.
Raghu.
^ permalink raw reply
* Re: [PATCH net-next v2 6/9] net: use core MTU range checking in virt drivers
From: Michael S. Tsirkin @ 2016-10-20 20:23 UTC (permalink / raw)
To: Jarod Wilson
Cc: linux-kernel, netdev, virtualization, K. Y. Srinivasan,
Haiyang Zhang, Shrikrishna Khare, VMware, Inc., Wei Liu,
Paul Durrant, David Kershner, Aaron Conole
In-Reply-To: <20161020175524.6184-7-jarod@redhat.com>
On Thu, Oct 20, 2016 at 01:55:21PM -0400, Jarod Wilson wrote:
> hyperv_net:
> - set min/max_mtu, per Haiyang, after rndis_filter_device_add
>
> virtio_net:
> - set min/max_mtu
> - remove virtnet_change_mtu
> vmxnet3:
> - set min/max_mtu
>
> xen-netback:
> - min_mtu = 0, max_mtu = 65517
>
> xen-netfront:
> - min_mtu = 0, max_mtu = 65535
>
> unisys/visor:
> - clean up defines a little to not clash with network core or add
> redundat definitions
>
> CC: netdev@vger.kernel.org
> CC: virtualization@lists.linux-foundation.org
> CC: "K. Y. Srinivasan" <kys@microsoft.com>
> CC: Haiyang Zhang <haiyangz@microsoft.com>
> CC: "Michael S. Tsirkin" <mst@redhat.com>
> CC: Shrikrishna Khare <skhare@vmware.com>
> CC: "VMware, Inc." <pv-drivers@vmware.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: David Kershner <david.kershner@unisys.com>
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
> drivers/net/hyperv/hyperv_net.h | 4 ++--
> drivers/net/hyperv/netvsc_drv.c | 14 +++++++-------
> drivers/net/virtio_net.c | 23 ++++++++++-------------
> drivers/net/vmxnet3/vmxnet3_drv.c | 7 ++++---
> drivers/net/xen-netback/interface.c | 5 ++++-
> drivers/net/xen-netfront.c | 2 ++
> drivers/staging/unisys/include/iochannel.h | 10 ++++------
> drivers/staging/unisys/visornic/visornic_main.c | 4 ++--
> 8 files changed, 35 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
> index f4fbcb5..3958ada 100644
> --- a/drivers/net/hyperv/hyperv_net.h
> +++ b/drivers/net/hyperv/hyperv_net.h
> @@ -606,8 +606,8 @@ struct nvsp_message {
> } __packed;
>
>
> -#define NETVSC_MTU 65536
> -#define NETVSC_MTU_MIN 68
> +#define NETVSC_MTU 65535
> +#define NETVSC_MTU_MIN ETH_MIN_MTU
>
> #define NETVSC_RECEIVE_BUFFER_SIZE (1024*1024*16) /* 16MB */
> #define NETVSC_RECEIVE_BUFFER_SIZE_LEGACY (1024*1024*15) /* 15MB */
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index f0919bd..3b28cf1 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -872,19 +872,12 @@ static int netvsc_change_mtu(struct net_device *ndev, int mtu)
> struct netvsc_device *nvdev = ndevctx->nvdev;
> struct hv_device *hdev = ndevctx->device_ctx;
> struct netvsc_device_info device_info;
> - int limit = ETH_DATA_LEN;
> u32 num_chn;
> int ret = 0;
>
> if (ndevctx->start_remove || !nvdev || nvdev->destroy)
> return -ENODEV;
>
> - if (nvdev->nvsp_version >= NVSP_PROTOCOL_VERSION_2)
> - limit = NETVSC_MTU - ETH_HLEN;
> -
> - if (mtu < NETVSC_MTU_MIN || mtu > limit)
> - return -EINVAL;
> -
> ret = netvsc_close(ndev);
> if (ret)
> goto out;
> @@ -1402,6 +1395,13 @@ static int netvsc_probe(struct hv_device *dev,
> netif_set_real_num_tx_queues(net, nvdev->num_chn);
> netif_set_real_num_rx_queues(net, nvdev->num_chn);
>
> + /* MTU range: 68 - 1500 or 65521 */
> + net->min_mtu = NETVSC_MTU_MIN;
> + if (nvdev->nvsp_version >= NVSP_PROTOCOL_VERSION_2)
> + net->max_mtu = NETVSC_MTU - ETH_HLEN;
> + else
> + net->max_mtu = ETH_DATA_LEN;
> +
> ret = register_netdev(net);
> if (ret != 0) {
> pr_err("Unable to register netdev.\n");
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index fad84f3..720809f 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1419,17 +1419,6 @@ static const struct ethtool_ops virtnet_ethtool_ops = {
> .set_settings = virtnet_set_settings,
> };
>
> -#define MIN_MTU 68
> -#define MAX_MTU 65535
> -
> -static int virtnet_change_mtu(struct net_device *dev, int new_mtu)
> -{
> - if (new_mtu < MIN_MTU || new_mtu > MAX_MTU)
> - return -EINVAL;
> - dev->mtu = new_mtu;
> - return 0;
> -}
> -
> static const struct net_device_ops virtnet_netdev = {
> .ndo_open = virtnet_open,
> .ndo_stop = virtnet_close,
> @@ -1437,7 +1426,6 @@ static const struct net_device_ops virtnet_netdev = {
> .ndo_validate_addr = eth_validate_addr,
> .ndo_set_mac_address = virtnet_set_mac_address,
> .ndo_set_rx_mode = virtnet_set_rx_mode,
> - .ndo_change_mtu = virtnet_change_mtu,
> .ndo_get_stats64 = virtnet_stats,
> .ndo_vlan_rx_add_vid = virtnet_vlan_rx_add_vid,
> .ndo_vlan_rx_kill_vid = virtnet_vlan_rx_kill_vid,
> @@ -1748,6 +1736,9 @@ static bool virtnet_validate_features(struct virtio_device *vdev)
> return true;
> }
>
> +#define MIN_MTU ETH_MIN_MTU
> +#define MAX_MTU ETH_MAX_MTU
> +
Can we drop these btw?
> static int virtnet_probe(struct virtio_device *vdev)
> {
> int i, err;
> @@ -1821,6 +1812,10 @@ static int virtnet_probe(struct virtio_device *vdev)
>
> dev->vlan_features = dev->features;
>
> + /* MTU range: 68 - 65535 */
> + dev->min_mtu = MIN_MTU;
> + dev->max_mtu = MAX_MTU;
> +
> /* Configuration may specify what MAC to use. Otherwise random. */
> if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC))
> virtio_cread_bytes(vdev,
> @@ -1875,8 +1870,10 @@ static int virtnet_probe(struct virtio_device *vdev)
> mtu = virtio_cread16(vdev,
> offsetof(struct virtio_net_config,
> mtu));
> - if (virtnet_change_mtu(dev, mtu))
> + if (mtu < dev->min_mtu || mtu > dev->max_mtu)
In fact the > max_mtu branch does not make sense since a 16 bit
value can't exceed MAX_MTU.
> __virtio_clear_bit(vdev, VIRTIO_NET_F_MTU);
> + else
> + dev->mtu = mtu;
> }
>
> if (vi->any_header_sg)
> diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c b/drivers/net/vmxnet3/vmxnet3_drv.c
> index b5554f2..0c36de1 100644
> --- a/drivers/net/vmxnet3/vmxnet3_drv.c
> +++ b/drivers/net/vmxnet3/vmxnet3_drv.c
> @@ -2969,9 +2969,6 @@ vmxnet3_change_mtu(struct net_device *netdev, int new_mtu)
> struct vmxnet3_adapter *adapter = netdev_priv(netdev);
> int err = 0;
>
> - if (new_mtu < VMXNET3_MIN_MTU || new_mtu > VMXNET3_MAX_MTU)
> - return -EINVAL;
> -
> netdev->mtu = new_mtu;
>
> /*
> @@ -3428,6 +3425,10 @@ vmxnet3_probe_device(struct pci_dev *pdev,
> vmxnet3_set_ethtool_ops(netdev);
> netdev->watchdog_timeo = 5 * HZ;
>
> + /* MTU range: 60 - 9000 */
> + netdev->min_mtu = VMXNET3_MIN_MTU;
> + netdev->max_mtu = VMXNET3_MAX_MTU;
> +
> INIT_WORK(&adapter->work, vmxnet3_reset_work);
> set_bit(VMXNET3_STATE_BIT_QUIESCED, &adapter->state);
>
> diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
> index 74dc2bf..e30ffd2 100644
> --- a/drivers/net/xen-netback/interface.c
> +++ b/drivers/net/xen-netback/interface.c
> @@ -302,7 +302,7 @@ static int xenvif_close(struct net_device *dev)
> static int xenvif_change_mtu(struct net_device *dev, int mtu)
> {
> struct xenvif *vif = netdev_priv(dev);
> - int max = vif->can_sg ? 65535 - VLAN_ETH_HLEN : ETH_DATA_LEN;
> + int max = vif->can_sg ? ETH_MAX_MTU - VLAN_ETH_HLEN : ETH_DATA_LEN;
>
> if (mtu > max)
> return -EINVAL;
> @@ -471,6 +471,9 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
>
> dev->tx_queue_len = XENVIF_QUEUE_LENGTH;
>
> + dev->min_mtu = 0;
> + dev->max_mtu = ETH_MAX_MTU - VLAN_ETH_HLEN;
> +
> /*
> * Initialise a dummy MAC address. We choose the numerically
> * largest non-broadcast address to prevent the address getting
> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> index e17879d..7d616b0 100644
> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
> @@ -1329,6 +1329,8 @@ static struct net_device *xennet_create_dev(struct xenbus_device *dev)
> netdev->features |= netdev->hw_features;
>
> netdev->ethtool_ops = &xennet_ethtool_ops;
> + netdev->min_mtu = 0;
> + netdev->max_mtu = XEN_NETIF_MAX_TX_SIZE;
> SET_NETDEV_DEV(netdev, &dev->dev);
>
> np->netdev = netdev;
> diff --git a/drivers/staging/unisys/include/iochannel.h b/drivers/staging/unisys/include/iochannel.h
> index cba4433..9081b3f 100644
> --- a/drivers/staging/unisys/include/iochannel.h
> +++ b/drivers/staging/unisys/include/iochannel.h
> @@ -113,12 +113,10 @@ enum net_types {
>
> };
>
> -#define ETH_HEADER_SIZE 14 /* size of ethernet header */
> -
> #define ETH_MIN_DATA_SIZE 46 /* minimum eth data size */
> -#define ETH_MIN_PACKET_SIZE (ETH_HEADER_SIZE + ETH_MIN_DATA_SIZE)
> +#define ETH_MIN_PACKET_SIZE (ETH_HLEN + ETH_MIN_DATA_SIZE)
>
> -#define ETH_MAX_MTU 16384 /* maximum data size */
> +#define VISOR_ETH_MAX_MTU 16384 /* maximum data size */
>
> #ifndef MAX_MACADDR_LEN
> #define MAX_MACADDR_LEN 6 /* number of bytes in MAC address */
> @@ -288,7 +286,7 @@ struct net_pkt_xmt {
> int len; /* full length of data in the packet */
> int num_frags; /* number of fragments in frags containing data */
> struct phys_info frags[MAX_PHYS_INFO]; /* physical page information */
> - char ethhdr[ETH_HEADER_SIZE]; /* the ethernet header */
> + char ethhdr[ETH_HLEN]; /* the ethernet header */
> struct {
> /* these are needed for csum at uisnic end */
> u8 valid; /* 1 = struct is valid - else ignore */
> @@ -323,7 +321,7 @@ struct net_pkt_xmtdone {
> */
> #define RCVPOST_BUF_SIZE 4032
> #define MAX_NET_RCV_CHAIN \
> - ((ETH_MAX_MTU + ETH_HEADER_SIZE + RCVPOST_BUF_SIZE - 1) \
> + ((VISOR_ETH_MAX_MTU + ETH_HLEN + RCVPOST_BUF_SIZE - 1) \
> / RCVPOST_BUF_SIZE)
>
> struct net_pkt_rcvpost {
> diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
> index 1367007..f8a584b 100644
> --- a/drivers/staging/unisys/visornic/visornic_main.c
> +++ b/drivers/staging/unisys/visornic/visornic_main.c
> @@ -791,7 +791,7 @@ visornic_xmit(struct sk_buff *skb, struct net_device *netdev)
> * pointing to
> */
> firstfraglen = skb->len - skb->data_len;
> - if (firstfraglen < ETH_HEADER_SIZE) {
> + if (firstfraglen < ETH_HLEN) {
> spin_unlock_irqrestore(&devdata->priv_lock, flags);
> devdata->busy_cnt++;
> dev_err(&netdev->dev,
> @@ -864,7 +864,7 @@ visornic_xmit(struct sk_buff *skb, struct net_device *netdev)
> /* copy ethernet header from first frag into ocmdrsp
> * - everything else will be pass in frags & DMA'ed
> */
> - memcpy(cmdrsp->net.xmt.ethhdr, skb->data, ETH_HEADER_SIZE);
> + memcpy(cmdrsp->net.xmt.ethhdr, skb->data, ETH_HLEN);
> /* copy frags info - from skb->data we need to only provide access
> * beyond eth header
> */
> --
> 2.10.0
^ permalink raw reply
* [PATCH net-next v5 0/3] udp: refactor memory accounting
From: Paolo Abeni @ 2016-10-20 20:31 UTC (permalink / raw)
To: netdev-u79uwXL29TY76Z2rM5mHXA
Cc: David S. Miller, James Morris, Trond Myklebust, Alexander Duyck,
Daniel Borkmann, Eric Dumazet, Tom Herbert, Hannes Frederic Sowa,
Edward Cree, linux-nfs-u79uwXL29TY76Z2rM5mHXA
This patch series refactor the udp memory accounting, replacing the
generic implementation with a custom one, in order to remove the needs for
locking the socket on the enqueue and dequeue operations. The socket backlog
usage is dropped, as well.
The first patch factor out pieces of some queue and memory management
socket helpers, so that they can later be used by the udp memory accounting
functions.
The second patch adds the memory account helpers, without using them.
The third patch replacse the old rx memory accounting path for udp over ipv4 and
udp over ipv6. In kernel UDP users are updated, as well.
The memory accounting schema is described in detail in the individual patch
commit message.
The performance gain depends on the specific scenario; with few flows (and
little contention in the original code) the differences are in the noise range,
while with several flows contending the same socket, the measured speed-up
is relevant (e.g. even over 100% in case of extreme contention)
v4 -> v5:
- use the receive queue spin lock to protect the memory accounting
- several minor clean-up
v3 -> v4:
- simplified the locking schema, always use a plain spinlock
v2 -> v3:
- do not set the now unsed backlog_rcv callback
v1 -> v2:
- changed slighly the memory accounting schema, we now perform lazy reclaim
- fixed forward_alloc updating issue
- fixed memory counter integer overflows
Paolo Abeni (3):
net/socket: factor out helpers for memory and queue manipulation
udp: implement memory accounting helpers
udp: use it's own memory accounting schema
include/net/sock.h | 4 ++
include/net/udp.h | 4 ++
net/core/datagram.c | 36 +++++++-----
net/core/sock.c | 64 ++++++++++++++-------
net/ipv4/udp.c | 151 ++++++++++++++++++++++++++++++++++++++------------
net/ipv6/udp.c | 34 +++---------
net/sunrpc/svcsock.c | 20 +++++--
net/sunrpc/xprtsock.c | 2 +-
8 files changed, 216 insertions(+), 99 deletions(-)
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" 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
* [PATCH net-next v5 2/3] udp: implement memory accounting helpers
From: Paolo Abeni @ 2016-10-20 20:31 UTC (permalink / raw)
To: netdev-u79uwXL29TY76Z2rM5mHXA
Cc: David S. Miller, James Morris, Trond Myklebust, Alexander Duyck,
Daniel Borkmann, Eric Dumazet, Tom Herbert, Hannes Frederic Sowa,
Edward Cree, linux-nfs-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <cover.1476979679.git.pabeni-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Avoid using the generic helpers.
Use the receive queue spin lock to protect the memory
accounting operation, both on enqueue and on dequeue.
On dequeue perform partial memory reclaiming, trying to
leave a quantum of forward allocated memory.
On enqueue use a custom helper, to allow some optimizations:
- use a plain spin_lock() variant instead of the slightly
costly spin_lock_irqsave(),
- avoid dst_force check, since the calling code has already
dropped the skb dst
The above needs custom memory reclaiming on shutdown, provided
by the udp_destruct_sock().
v4 -> v5:
- replace the mem_lock with the receive queue spin lock
- ensure that the bh is always allowed to enqueue at least
a skb, even if sk_rcvbuf is exceeded
v3 -> v4:
- reworked memory accunting, simplifying the schema
- provide an helper for both memory scheduling and enqueuing
v1 -> v2:
- use a udp specific destrctor to perform memory reclaiming
- remove a couple of helpers, unneeded after the above cleanup
- do not reclaim memory on dequeue if not under memory
pressure
- reworked the fwd accounting schema to avoid potential
integer overflow
Acked-by: Hannes Frederic Sowa <hannes-tFNcAqjVMyqKXQKiL6tip0B+6BGkLq7r@public.gmane.org>
Signed-off-by: Paolo Abeni <pabeni-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
include/net/udp.h | 4 ++
net/ipv4/udp.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 112 insertions(+)
diff --git a/include/net/udp.h b/include/net/udp.h
index ea53a87..18f1e6b 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -246,6 +246,9 @@ static inline __be16 udp_flow_src_port(struct net *net, struct sk_buff *skb,
}
/* net/ipv4/udp.c */
+void skb_consume_udp(struct sock *sk, struct sk_buff *skb, int len);
+int __udp_enqueue_schedule_skb(struct sock *sk, struct sk_buff *skb);
+
void udp_v4_early_demux(struct sk_buff *skb);
int udp_get_port(struct sock *sk, unsigned short snum,
int (*saddr_cmp)(const struct sock *,
@@ -258,6 +261,7 @@ int udp_get_port(struct sock *sk, unsigned short snum,
void udp4_hwcsum(struct sk_buff *skb, __be32 src, __be32 dst);
int udp_rcv(struct sk_buff *skb);
int udp_ioctl(struct sock *sk, int cmd, unsigned long arg);
+int udp_init_sock(struct sock *sk);
int udp_disconnect(struct sock *sk, int flags);
unsigned int udp_poll(struct file *file, struct socket *sock, poll_table *wait);
struct sk_buff *skb_udp_tunnel_segment(struct sk_buff *skb,
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 7d96dc2..5e79992 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1172,6 +1172,114 @@ int udp_sendpage(struct sock *sk, struct page *page, int offset,
return ret;
}
+static void udp_rmem_release(struct sock *sk, int size, int partial)
+{
+ int amt;
+
+ atomic_sub(size, &sk->sk_rmem_alloc);
+
+ spin_lock_bh(&sk->sk_receive_queue.lock);
+ sk->sk_forward_alloc += size;
+ amt = (sk->sk_forward_alloc - partial) & ~(SK_MEM_QUANTUM - 1);
+ sk->sk_forward_alloc -= amt;
+ spin_unlock_bh(&sk->sk_receive_queue.lock);
+
+ if (amt)
+ __sk_mem_reduce_allocated(sk, amt >> SK_MEM_QUANTUM_SHIFT);
+}
+
+static void udp_rmem_free(struct sk_buff *skb)
+{
+ udp_rmem_release(skb->sk, skb->truesize, 1);
+}
+
+int __udp_enqueue_schedule_skb(struct sock *sk, struct sk_buff *skb)
+{
+ struct sk_buff_head *list = &sk->sk_receive_queue;
+ int rmem, delta, amt, err = -ENOMEM;
+ int size = skb->truesize;
+
+ /* try to avoid the costly atomic add/sub pair when the receive
+ * queue is full; always allow at least a packet
+ */
+ rmem = atomic_read(&sk->sk_rmem_alloc);
+ if (rmem && (rmem + size > sk->sk_rcvbuf))
+ goto drop;
+
+ /* we drop only if the receive buf is full and the receive
+ * queue contains some other skb
+ */
+ rmem = atomic_add_return(size, &sk->sk_rmem_alloc);
+ if ((rmem > sk->sk_rcvbuf) && (rmem > size))
+ goto uncharge_drop;
+
+ skb_orphan(skb);
+
+ spin_lock(&list->lock);
+ if (size >= sk->sk_forward_alloc) {
+ amt = sk_mem_pages(size);
+ delta = amt << SK_MEM_QUANTUM_SHIFT;
+ if (!__sk_mem_raise_allocated(sk, delta, amt, SK_MEM_RECV)) {
+ err = -ENOBUFS;
+ spin_unlock(&list->lock);
+ goto uncharge_drop;
+ }
+
+ sk->sk_forward_alloc += delta;
+ }
+
+ sk->sk_forward_alloc -= size;
+
+ /* the skb owner in now the udp socket */
+ skb->sk = sk;
+ skb->destructor = udp_rmem_free;
+ skb->dev = NULL;
+ sock_skb_set_dropcount(sk, skb);
+
+ __skb_queue_tail(list, skb);
+ spin_unlock(&list->lock);
+
+ if (!sock_flag(sk, SOCK_DEAD))
+ sk->sk_data_ready(sk);
+
+ return 0;
+
+uncharge_drop:
+ atomic_sub(skb->truesize, &sk->sk_rmem_alloc);
+
+drop:
+ atomic_inc(&sk->sk_drops);
+ return err;
+}
+EXPORT_SYMBOL_GPL(__udp_enqueue_schedule_skb);
+
+static void udp_destruct_sock(struct sock *sk)
+{
+ /* reclaim completely the forward allocated memory */
+ __skb_queue_purge(&sk->sk_receive_queue);
+ udp_rmem_release(sk, 0, 0);
+ inet_sock_destruct(sk);
+}
+
+int udp_init_sock(struct sock *sk)
+{
+ sk->sk_destruct = udp_destruct_sock;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(udp_init_sock);
+
+void skb_consume_udp(struct sock *sk, struct sk_buff *skb, int len)
+{
+ if (unlikely(READ_ONCE(sk->sk_peek_off) >= 0)) {
+ bool slow = lock_sock_fast(sk);
+
+ sk_peek_offset_bwd(sk, len);
+ unlock_sock_fast(sk, slow);
+ }
+ consume_skb(skb);
+}
+EXPORT_SYMBOL_GPL(skb_consume_udp);
+
/**
* first_packet_length - return length of first packet in receive queue
* @sk: socket
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" 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 related
* [PATCH net-next v5 3/3] udp: use it's own memory accounting schema
From: Paolo Abeni @ 2016-10-20 20:31 UTC (permalink / raw)
To: netdev-u79uwXL29TY76Z2rM5mHXA
Cc: David S. Miller, James Morris, Trond Myklebust, Alexander Duyck,
Daniel Borkmann, Eric Dumazet, Tom Herbert, Hannes Frederic Sowa,
Edward Cree, linux-nfs-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <cover.1476979679.git.pabeni-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Completely avoid default sock memory accounting and replace it
with udp-specific accounting.
Since the new memory accounting model encapsulates completely
the required locking, remove the socket lock on both enqueue and
dequeue, and avoid using the backlog on enqueue.
Be sure to clean-up rx queue memory on socket destruction, using
udp its own sk_destruct.
Tested using pktgen with random src port, 64 bytes packet,
wire-speed on a 10G link as sender and udp_sink as the receiver,
using an l4 tuple rxhash to stress the contention, and one or more
udp_sink instances with reuseport.
nr readers Kpps (vanilla) Kpps (patched)
1 170 440
3 1250 2150
6 3000 3650
9 4200 4450
12 5700 6250
v4 -> v5:
- avoid unneeded test in first_packet_length
v3 -> v4:
- remove useless sk_rcvqueues_full() call
v2 -> v3:
- do not set the now unsed backlog_rcv callback
v1 -> v2:
- add memory pressure support
- fixed dropwatch accounting for ipv6
Acked-by: Hannes Frederic Sowa <hannes-tFNcAqjVMyqKXQKiL6tip0B+6BGkLq7r@public.gmane.org>
Signed-off-by: Paolo Abeni <pabeni-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
net/ipv4/udp.c | 43 ++++++++-----------------------------------
net/ipv6/udp.c | 34 ++++++++--------------------------
net/sunrpc/svcsock.c | 20 ++++++++++++++++----
net/sunrpc/xprtsock.c | 2 +-
4 files changed, 33 insertions(+), 66 deletions(-)
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 5e79992..73d33a8 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1309,13 +1309,7 @@ static int first_packet_length(struct sock *sk)
res = skb ? skb->len : -1;
spin_unlock_bh(&rcvq->lock);
- if (!skb_queue_empty(&list_kill)) {
- bool slow = lock_sock_fast(sk);
-
- __skb_queue_purge(&list_kill);
- sk_mem_reclaim_partial(sk);
- unlock_sock_fast(sk, slow);
- }
+ __skb_queue_purge(&list_kill);
return res;
}
@@ -1364,7 +1358,6 @@ int udp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int noblock,
int err;
int is_udplite = IS_UDPLITE(sk);
bool checksum_valid = false;
- bool slow;
if (flags & MSG_ERRQUEUE)
return ip_recv_error(sk, msg, len, addr_len);
@@ -1405,13 +1398,12 @@ int udp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int noblock,
}
if (unlikely(err)) {
- trace_kfree_skb(skb, udp_recvmsg);
if (!peeked) {
atomic_inc(&sk->sk_drops);
UDP_INC_STATS(sock_net(sk),
UDP_MIB_INERRORS, is_udplite);
}
- skb_free_datagram_locked(sk, skb);
+ kfree_skb(skb);
return err;
}
@@ -1436,16 +1428,15 @@ int udp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int noblock,
if (flags & MSG_TRUNC)
err = ulen;
- __skb_free_datagram_locked(sk, skb, peeking ? -err : err);
+ skb_consume_udp(sk, skb, peeking ? -err : err);
return err;
csum_copy_err:
- slow = lock_sock_fast(sk);
- if (!skb_kill_datagram(sk, skb, flags)) {
+ if (!__sk_queue_drop_skb(sk, skb, flags)) {
UDP_INC_STATS(sock_net(sk), UDP_MIB_CSUMERRORS, is_udplite);
UDP_INC_STATS(sock_net(sk), UDP_MIB_INERRORS, is_udplite);
}
- unlock_sock_fast(sk, slow);
+ kfree_skb(skb);
/* starting over for a new packet, but check if we need to yield */
cond_resched();
@@ -1564,7 +1555,7 @@ static int __udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
sk_incoming_cpu_update(sk);
}
- rc = __sock_queue_rcv_skb(sk, skb);
+ rc = __udp_enqueue_schedule_skb(sk, skb);
if (rc < 0) {
int is_udplite = IS_UDPLITE(sk);
@@ -1579,7 +1570,6 @@ static int __udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
}
return 0;
-
}
static struct static_key udp_encap_needed __read_mostly;
@@ -1601,7 +1591,6 @@ void udp_encap_enable(void)
int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
{
struct udp_sock *up = udp_sk(sk);
- int rc;
int is_udplite = IS_UDPLITE(sk);
/*
@@ -1688,25 +1677,9 @@ int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
goto drop;
udp_csum_pull_header(skb);
- if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
- __UDP_INC_STATS(sock_net(sk), UDP_MIB_RCVBUFERRORS,
- is_udplite);
- goto drop;
- }
-
- rc = 0;
ipv4_pktinfo_prepare(sk, skb);
- bh_lock_sock(sk);
- if (!sock_owned_by_user(sk))
- rc = __udp_queue_rcv_skb(sk, skb);
- else if (sk_add_backlog(sk, skb, sk->sk_rcvbuf)) {
- bh_unlock_sock(sk);
- goto drop;
- }
- bh_unlock_sock(sk);
-
- return rc;
+ return __udp_queue_rcv_skb(sk, skb);
csum_error:
__UDP_INC_STATS(sock_net(sk), UDP_MIB_CSUMERRORS, is_udplite);
@@ -2316,13 +2289,13 @@ struct proto udp_prot = {
.connect = ip4_datagram_connect,
.disconnect = udp_disconnect,
.ioctl = udp_ioctl,
+ .init = udp_init_sock,
.destroy = udp_destroy_sock,
.setsockopt = udp_setsockopt,
.getsockopt = udp_getsockopt,
.sendmsg = udp_sendmsg,
.recvmsg = udp_recvmsg,
.sendpage = udp_sendpage,
- .backlog_rcv = __udp_queue_rcv_skb,
.release_cb = ip4_datagram_release_cb,
.hash = udp_lib_hash,
.unhash = udp_lib_unhash,
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 9aa7c1c..71963b2 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -334,7 +334,6 @@ int udpv6_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
int is_udplite = IS_UDPLITE(sk);
bool checksum_valid = false;
int is_udp4;
- bool slow;
if (flags & MSG_ERRQUEUE)
return ipv6_recv_error(sk, msg, len, addr_len);
@@ -378,7 +377,6 @@ int udpv6_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
goto csum_copy_err;
}
if (unlikely(err)) {
- trace_kfree_skb(skb, udpv6_recvmsg);
if (!peeked) {
atomic_inc(&sk->sk_drops);
if (is_udp4)
@@ -388,7 +386,7 @@ int udpv6_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
UDP6_INC_STATS(sock_net(sk), UDP_MIB_INERRORS,
is_udplite);
}
- skb_free_datagram_locked(sk, skb);
+ kfree_skb(skb);
return err;
}
if (!peeked) {
@@ -437,12 +435,11 @@ int udpv6_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
if (flags & MSG_TRUNC)
err = ulen;
- __skb_free_datagram_locked(sk, skb, peeking ? -err : err);
+ skb_consume_udp(sk, skb, peeking ? -err : err);
return err;
csum_copy_err:
- slow = lock_sock_fast(sk);
- if (!skb_kill_datagram(sk, skb, flags)) {
+ if (!__sk_queue_drop_skb(sk, skb, flags)) {
if (is_udp4) {
UDP_INC_STATS(sock_net(sk),
UDP_MIB_CSUMERRORS, is_udplite);
@@ -455,7 +452,7 @@ int udpv6_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
UDP_MIB_INERRORS, is_udplite);
}
}
- unlock_sock_fast(sk, slow);
+ kfree_skb(skb);
/* starting over for a new packet, but check if we need to yield */
cond_resched();
@@ -523,7 +520,7 @@ static int __udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
sk_incoming_cpu_update(sk);
}
- rc = __sock_queue_rcv_skb(sk, skb);
+ rc = __udp_enqueue_schedule_skb(sk, skb);
if (rc < 0) {
int is_udplite = IS_UDPLITE(sk);
@@ -535,6 +532,7 @@ static int __udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
kfree_skb(skb);
return -1;
}
+
return 0;
}
@@ -556,7 +554,6 @@ void udpv6_encap_enable(void)
int udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
{
struct udp_sock *up = udp_sk(sk);
- int rc;
int is_udplite = IS_UDPLITE(sk);
if (!xfrm6_policy_check(sk, XFRM_POLICY_IN, skb))
@@ -622,25 +619,10 @@ int udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
goto drop;
udp_csum_pull_header(skb);
- if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
- __UDP6_INC_STATS(sock_net(sk),
- UDP_MIB_RCVBUFERRORS, is_udplite);
- goto drop;
- }
skb_dst_drop(skb);
- bh_lock_sock(sk);
- rc = 0;
- if (!sock_owned_by_user(sk))
- rc = __udpv6_queue_rcv_skb(sk, skb);
- else if (sk_add_backlog(sk, skb, sk->sk_rcvbuf)) {
- bh_unlock_sock(sk);
- goto drop;
- }
- bh_unlock_sock(sk);
-
- return rc;
+ return __udpv6_queue_rcv_skb(sk, skb);
csum_error:
__UDP6_INC_STATS(sock_net(sk), UDP_MIB_CSUMERRORS, is_udplite);
@@ -1433,12 +1415,12 @@ struct proto udpv6_prot = {
.connect = ip6_datagram_connect,
.disconnect = udp_disconnect,
.ioctl = udp_ioctl,
+ .init = udp_init_sock,
.destroy = udpv6_destroy_sock,
.setsockopt = udpv6_setsockopt,
.getsockopt = udpv6_getsockopt,
.sendmsg = udpv6_sendmsg,
.recvmsg = udpv6_recvmsg,
- .backlog_rcv = __udpv6_queue_rcv_skb,
.release_cb = ip6_datagram_release_cb,
.hash = udp_lib_hash,
.unhash = udp_lib_unhash,
diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 57625f6..e2a55dc 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -39,6 +39,7 @@
#include <net/checksum.h>
#include <net/ip.h>
#include <net/ipv6.h>
+#include <net/udp.h>
#include <net/tcp.h>
#include <net/tcp_states.h>
#include <asm/uaccess.h>
@@ -129,6 +130,18 @@ static void svc_release_skb(struct svc_rqst *rqstp)
}
}
+static void svc_release_udp_skb(struct svc_rqst *rqstp)
+{
+ struct sk_buff *skb = rqstp->rq_xprt_ctxt;
+
+ if (skb) {
+ rqstp->rq_xprt_ctxt = NULL;
+
+ dprintk("svc: service %p, releasing skb %p\n", rqstp, skb);
+ consume_skb(skb);
+ }
+}
+
union svc_pktinfo_u {
struct in_pktinfo pkti;
struct in6_pktinfo pkti6;
@@ -575,7 +588,7 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
goto out_free;
}
local_bh_enable();
- skb_free_datagram_locked(svsk->sk_sk, skb);
+ consume_skb(skb);
} else {
/* we can use it in-place */
rqstp->rq_arg.head[0].iov_base = skb->data;
@@ -602,8 +615,7 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
return len;
out_free:
- trace_kfree_skb(skb, svc_udp_recvfrom);
- skb_free_datagram_locked(svsk->sk_sk, skb);
+ kfree_skb(skb);
return 0;
}
@@ -660,7 +672,7 @@ static struct svc_xprt *svc_udp_create(struct svc_serv *serv,
.xpo_create = svc_udp_create,
.xpo_recvfrom = svc_udp_recvfrom,
.xpo_sendto = svc_udp_sendto,
- .xpo_release_rqst = svc_release_skb,
+ .xpo_release_rqst = svc_release_udp_skb,
.xpo_detach = svc_sock_detach,
.xpo_free = svc_sock_free,
.xpo_prep_reply_hdr = svc_udp_prep_reply_hdr,
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index bf16883..8e04a43 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1074,7 +1074,7 @@ static void xs_udp_data_receive(struct sock_xprt *transport)
skb = skb_recv_datagram(sk, 0, 1, &err);
if (skb != NULL) {
xs_udp_data_read_skb(&transport->xprt, sk, skb);
- skb_free_datagram_locked(sk, skb);
+ consume_skb(skb);
continue;
}
if (!test_and_clear_bit(XPRT_SOCK_DATA_READY, &transport->sock_state))
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" 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 related
* [PATCH net-next v5 1/3] net/socket: factor out helpers for memory and queue manipulation
From: Paolo Abeni @ 2016-10-20 20:31 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, James Morris, Trond Myklebust, Alexander Duyck,
Daniel Borkmann, Eric Dumazet, Tom Herbert, Hannes Frederic Sowa,
Edward Cree, linux-nfs
In-Reply-To: <cover.1476979679.git.pabeni@redhat.com>
Basic sock operations that udp code can use with its own
memory accounting schema. No functional change is introduced
in the existing APIs.
v4 -> v5:
- avoid whitespace changes
v2 -> v4:
- avoid exporting __sock_enqueue_skb
v1 -> v2:
- avoid export sock_rmem_free
Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
include/net/sock.h | 4 ++++
net/core/datagram.c | 36 ++++++++++++++++++------------
net/core/sock.c | 64 +++++++++++++++++++++++++++++++++++++----------------
3 files changed, 71 insertions(+), 33 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index ebf75db..2764895 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1274,7 +1274,9 @@ static inline struct inode *SOCK_INODE(struct socket *socket)
/*
* Functions for memory accounting
*/
+int __sk_mem_raise_allocated(struct sock *sk, int size, int amt, int kind);
int __sk_mem_schedule(struct sock *sk, int size, int kind);
+void __sk_mem_reduce_allocated(struct sock *sk, int amount);
void __sk_mem_reclaim(struct sock *sk, int amount);
#define SK_MEM_QUANTUM ((int)PAGE_SIZE)
@@ -1950,6 +1952,8 @@ void sk_reset_timer(struct sock *sk, struct timer_list *timer,
void sk_stop_timer(struct sock *sk, struct timer_list *timer);
+int __sk_queue_drop_skb(struct sock *sk, struct sk_buff *skb,
+ unsigned int flags);
int __sock_queue_rcv_skb(struct sock *sk, struct sk_buff *skb);
int sock_queue_rcv_skb(struct sock *sk, struct sk_buff *skb);
diff --git a/net/core/datagram.c b/net/core/datagram.c
index b7de71f..bfb973a 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c
@@ -323,6 +323,27 @@ void __skb_free_datagram_locked(struct sock *sk, struct sk_buff *skb, int len)
}
EXPORT_SYMBOL(__skb_free_datagram_locked);
+int __sk_queue_drop_skb(struct sock *sk, struct sk_buff *skb,
+ unsigned int flags)
+{
+ int err = 0;
+
+ if (flags & MSG_PEEK) {
+ err = -ENOENT;
+ spin_lock_bh(&sk->sk_receive_queue.lock);
+ if (skb == skb_peek(&sk->sk_receive_queue)) {
+ __skb_unlink(skb, &sk->sk_receive_queue);
+ atomic_dec(&skb->users);
+ err = 0;
+ }
+ spin_unlock_bh(&sk->sk_receive_queue.lock);
+ }
+
+ atomic_inc(&sk->sk_drops);
+ return err;
+}
+EXPORT_SYMBOL(__sk_queue_drop_skb);
+
/**
* skb_kill_datagram - Free a datagram skbuff forcibly
* @sk: socket
@@ -346,23 +367,10 @@ void __skb_free_datagram_locked(struct sock *sk, struct sk_buff *skb, int len)
int skb_kill_datagram(struct sock *sk, struct sk_buff *skb, unsigned int flags)
{
- int err = 0;
-
- if (flags & MSG_PEEK) {
- err = -ENOENT;
- spin_lock_bh(&sk->sk_receive_queue.lock);
- if (skb == skb_peek(&sk->sk_receive_queue)) {
- __skb_unlink(skb, &sk->sk_receive_queue);
- atomic_dec(&skb->users);
- err = 0;
- }
- spin_unlock_bh(&sk->sk_receive_queue.lock);
- }
+ int err = __sk_queue_drop_skb(sk, skb, flags);
kfree_skb(skb);
- atomic_inc(&sk->sk_drops);
sk_mem_reclaim_partial(sk);
-
return err;
}
EXPORT_SYMBOL(skb_kill_datagram);
diff --git a/net/core/sock.c b/net/core/sock.c
index c73e28f..d8e4532e 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2091,24 +2091,18 @@ int sk_wait_data(struct sock *sk, long *timeo, const struct sk_buff *skb)
EXPORT_SYMBOL(sk_wait_data);
/**
- * __sk_mem_schedule - increase sk_forward_alloc and memory_allocated
+ * __sk_mem_raise_allocated - increase memory_allocated
* @sk: socket
* @size: memory size to allocate
+ * @amt: pages to allocate
* @kind: allocation type
*
- * If kind is SK_MEM_SEND, it means wmem allocation. Otherwise it means
- * rmem allocation. This function assumes that protocols which have
- * memory_pressure use sk_wmem_queued as write buffer accounting.
+ * Similar to __sk_mem_schedule(), but does not update sk_forward_alloc
*/
-int __sk_mem_schedule(struct sock *sk, int size, int kind)
+int __sk_mem_raise_allocated(struct sock *sk, int size, int amt, int kind)
{
struct proto *prot = sk->sk_prot;
- int amt = sk_mem_pages(size);
- long allocated;
-
- sk->sk_forward_alloc += amt * SK_MEM_QUANTUM;
-
- allocated = sk_memory_allocated_add(sk, amt);
+ long allocated = sk_memory_allocated_add(sk, amt);
if (mem_cgroup_sockets_enabled && sk->sk_memcg &&
!mem_cgroup_charge_skmem(sk->sk_memcg, amt))
@@ -2169,9 +2163,6 @@ int __sk_mem_schedule(struct sock *sk, int size, int kind)
trace_sock_exceed_buf_limit(sk, prot, allocated);
- /* Alas. Undo changes. */
- sk->sk_forward_alloc -= amt * SK_MEM_QUANTUM;
-
sk_memory_allocated_sub(sk, amt);
if (mem_cgroup_sockets_enabled && sk->sk_memcg)
@@ -2179,18 +2170,40 @@ int __sk_mem_schedule(struct sock *sk, int size, int kind)
return 0;
}
+EXPORT_SYMBOL(__sk_mem_raise_allocated);
+
+/**
+ * __sk_mem_schedule - increase sk_forward_alloc and memory_allocated
+ * @sk: socket
+ * @size: memory size to allocate
+ * @kind: allocation type
+ *
+ * If kind is SK_MEM_SEND, it means wmem allocation. Otherwise it means
+ * rmem allocation. This function assumes that protocols which have
+ * memory_pressure use sk_wmem_queued as write buffer accounting.
+ */
+int __sk_mem_schedule(struct sock *sk, int size, int kind)
+{
+ int ret, amt = sk_mem_pages(size);
+
+ sk->sk_forward_alloc += amt << SK_MEM_QUANTUM_SHIFT;
+ ret = __sk_mem_raise_allocated(sk, size, amt, kind);
+ if (!ret)
+ sk->sk_forward_alloc -= amt << SK_MEM_QUANTUM_SHIFT;
+ return ret;
+}
EXPORT_SYMBOL(__sk_mem_schedule);
/**
- * __sk_mem_reclaim - reclaim memory_allocated
+ * __sk_mem_reduce_allocated - reclaim memory_allocated
* @sk: socket
- * @amount: number of bytes (rounded down to a SK_MEM_QUANTUM multiple)
+ * @amount: number of quanta
+ *
+ * Similar to __sk_mem_reclaim(), but does not update sk_forward_alloc
*/
-void __sk_mem_reclaim(struct sock *sk, int amount)
+void __sk_mem_reduce_allocated(struct sock *sk, int amount)
{
- amount >>= SK_MEM_QUANTUM_SHIFT;
sk_memory_allocated_sub(sk, amount);
- sk->sk_forward_alloc -= amount << SK_MEM_QUANTUM_SHIFT;
if (mem_cgroup_sockets_enabled && sk->sk_memcg)
mem_cgroup_uncharge_skmem(sk->sk_memcg, amount);
@@ -2199,6 +2212,19 @@ void __sk_mem_reclaim(struct sock *sk, int amount)
(sk_memory_allocated(sk) < sk_prot_mem_limits(sk, 0)))
sk_leave_memory_pressure(sk);
}
+EXPORT_SYMBOL(__sk_mem_reduce_allocated);
+
+/**
+ * __sk_mem_reclaim - reclaim sk_forward_alloc and memory_allocated
+ * @sk: socket
+ * @amount: number of bytes (rounded down to a SK_MEM_QUANTUM multiple)
+ */
+void __sk_mem_reclaim(struct sock *sk, int amount)
+{
+ amount >>= SK_MEM_QUANTUM_SHIFT;
+ sk->sk_forward_alloc -= amount << SK_MEM_QUANTUM_SHIFT;
+ __sk_mem_reduce_allocated(sk, amount);
+}
EXPORT_SYMBOL(__sk_mem_reclaim);
int sk_set_peek_off(struct sock *sk, int val)
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH net] ipv4: disable BH in set_ping_group_range()
From: Eric Dumazet @ 2016-10-20 20:43 UTC (permalink / raw)
To: Cong Wang; +Cc: David Miller, netdev, Eric Salo
In-Reply-To: <CAM_iQpV-00gemayMkdLGzb8wo8CNimOZQHDXrvH1jeN6Svrs8w@mail.gmail.com>
On Thu, 2016-10-20 at 12:44 -0700, Cong Wang wrote:
> On Thu, Oct 20, 2016 at 12:40 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > On Thu, Oct 20, 2016 at 12:32 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >> On Thu, Oct 20, 2016 at 10:26 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >>> From: Eric Dumazet <edumazet@google.com>
> >>>
> >>> In commit 4ee3bd4a8c746 ("ipv4: disable BH when changing ip local port
> >>> range") Cong added BH protection in set_local_port_range() but missed
> >>> that same fix was needed in set_ping_group_range()
> >>
> >> Don't know why ping_group_range shares the same lock with local_port_range...
> >> Perhaps just for saving a few bytes, but that is why I missed this place.
> >
> > Hold on... We clearly have typos there... Your fix is not correct.
>
> We need the attached patch, your patch should be reverted, because
> unlike local_port_range we never read it in BH context, no need to bother _bh.
Well, we do not change this sysctl very often, so I am not sure why we
need different seqlocks to protect these ranges.
Seems a waste of space really (per netns)
^ permalink raw reply
* Re: [PATCH net] udp: must lock the socket in udp_disconnect()
From: Eric Dumazet @ 2016-10-20 20:44 UTC (permalink / raw)
To: David Miller; +Cc: sploving1, netdev
In-Reply-To: <20161020.144610.532749573029633695.davem@davemloft.net>
On Thu, 2016-10-20 at 14:46 -0400, David Miller wrote:
>
> Applied, sounds like I should queue this up for -stable too right?
Yes, I believe all stable versions have this bug.
Thanks.
^ permalink raw reply
* Re: [PATCH net-next V2 1/9] liquidio CN23XX: HW config for VF support
From: David Miller @ 2016-10-20 20:57 UTC (permalink / raw)
To: Raghu.Vatsavayi; +Cc: netdev, Derek.Chickles, Satananda.Burla, Felix.Manlunas
In-Reply-To: <DM3PR07MB21387F2B6D44558DBC36649881D50@DM3PR07MB2138.namprd07.prod.outlook.com>
From: "Vatsavayi, Raghu" <Raghu.Vatsavayi@cavium.com>
Date: Thu, 20 Oct 2016 20:01:37 +0000
>
>
>> -----Original Message-----
>> From: David Miller [mailto:davem@davemloft.net]
>> Sent: Thursday, October 20, 2016 11:13 AM
>> To: Vatsavayi, Raghu
>> Cc: netdev@vger.kernel.org; Vatsavayi, Raghu; Chickles, Derek; Burla,
>> Satananda; Manlunas, Felix
>> Subject: Re: [PATCH net-next V2 1/9] liquidio CN23XX: HW config for VF
>> support
>>
>> From: Raghu Vatsavayi <rvatsavayi@caviumnetworks.com>
>> Date: Wed, 19 Oct 2016 22:40:38 -0700
>>
>> > +/* Default behaviour of Liquidio is to provide one queue per VF. But
>> > +Liquidio
>> > + * can also provide multiple queues to each VF. If user wants to
>> > +change the
>> > + * default behaviour HW should be provided configuration info at init
>> > +time,
>> > + * based on which it will create control queues for communicating with
>> FW.
>> > + */
>> > +static u32 max_vfs[2] = { 0, 0 };
>> > +module_param_array(max_vfs, int, NULL, 0444);
>> > +MODULE_PARM_DESC(max_vfs, "Assign two comma-separated unsigned
>> > +integers that specify max number of VFs for PF0 (left of the comma)
>> > +and PF1 (right of the comma); for 23xx only. By default HW will
>> > +configure as many VFs as queues after allocating PF queues.To
>> > +increase queues for VF use this parameter. Use sysfs to create these
>> > +VFs.");
>> > +
>> > +static unsigned int num_queues_per_pf[2] = { 0, 0 };
>> > +module_param_array(num_queues_per_pf, uint, NULL, 0444);
>> > +MODULE_PARM_DESC(num_queues_per_pf, "two comma-separated
>> unsigned
>> > +integers that specify number of queues per PF0 (left of the comma)
>> > +and PF1 (right of the comma); for 23xx only");
>> > +
>> > static int ptp_enable = 1;
>>
>> We cannot continue to allow drivers to add custom module parameters to
>> control this. It is the worst user experience possible.
>>
>> We need a tree-wide generic, consistent, manner in which to configure and
>> control this kind of thing.
>
> Sure Dave, I will remove max_vfs module parameter and will use tree wide generic
> sysfs interface to enable VFs.
That's not what I meant.
I mean there needs to be a generic mechanism that isn't a per-device
knob (be it a module parameter or a sysctl, to me these are identical
functionality and user experience wise).
Something like ethtool or netlink.
^ permalink raw reply
* Re: [PATCH net] ipv4: disable BH in set_ping_group_range()
From: Cong Wang @ 2016-10-20 21:00 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Eric Salo
In-Reply-To: <1476996209.7065.28.camel@edumazet-glaptop3.roam.corp.google.com>
On Thu, Oct 20, 2016 at 1:43 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2016-10-20 at 12:44 -0700, Cong Wang wrote:
>> On Thu, Oct 20, 2016 at 12:40 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> > On Thu, Oct 20, 2016 at 12:32 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> >> On Thu, Oct 20, 2016 at 10:26 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> >>> From: Eric Dumazet <edumazet@google.com>
>> >>>
>> >>> In commit 4ee3bd4a8c746 ("ipv4: disable BH when changing ip local port
>> >>> range") Cong added BH protection in set_local_port_range() but missed
>> >>> that same fix was needed in set_ping_group_range()
>> >>
>> >> Don't know why ping_group_range shares the same lock with local_port_range...
>> >> Perhaps just for saving a few bytes, but that is why I missed this place.
>> >
>> > Hold on... We clearly have typos there... Your fix is not correct.
>>
>> We need the attached patch, your patch should be reverted, because
>> unlike local_port_range we never read it in BH context, no need to bother _bh.
>
> Well, we do not change this sysctl very often, so I am not sure why we
> need different seqlocks to protect these ranges.
>
> Seems a waste of space really (per netns)
Error prone vs. space saving, it's up to you...
But clearly current code is still broken even after your patch. I will send
a revert + previous typo fix.
^ 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