* Re: [PATCH 18/18] netvsc: call netif_receive_skb
From: Eric Dumazet @ 2017-01-24 23:07 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: davem, kys, netdev, Stephen Hemminger
In-Reply-To: <20170124150028.4981db42@xeon-e3>
On Tue, 2017-01-24 at 15:00 -0800, Stephen Hemminger wrote:
> On Tue, 24 Jan 2017 14:39:19 -0800
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> > On Tue, 2017-01-24 at 13:06 -0800, Stephen Hemminger wrote:
> > > To improve performance, netvsc can call network stack directly and
> > > avoid the local backlog queue. This is safe since incoming packets are
> > > handled in softirq context already because the receive function
> > > callback is called from a tasklet.
> >
> > Is this tasklet implementing a limit or something ?
>
> The ring only holds a fixed amount of data so there is a limit but
> it is quite large.
>
> >
> > netif_rx() queues packets to the backlog, which is processed later by
> > net_rx_action() like other NAPI, with limit of 64 packets per round.
>
> Since netvsc_receive has to copy all incoming data it is a bottleneck
> unto itself. By the time net_rx_action is invoked the cache is stale.
>
> >
> > Calling netif_receive_skb() means you can escape this ability to fairly
> > distribute the cpu cycles among multiple NAPI.
> >
> > I do not see range_cnt being capped in netvsc_receive()
>
> There is no cap. NAPI is coming and will help.
This was my point really.
If you call netif_receive_skb() in a loop, it is not NAPI anymore,
and it is a potential latency spike point, while blocking BH and not
servicing other queues depending on this cpu.
(While sofirqs processing NAPI (including netif_rc()) can be scheduled
to ksoftirqd)
Not a big deal, I only want to point out that netif_receive_skb() can be
dangerous if used in a loop.
^ permalink raw reply
* Re: [PATCH 18/18] netvsc: call netif_receive_skb
From: Stephen Hemminger @ 2017-01-24 23:00 UTC (permalink / raw)
To: Eric Dumazet; +Cc: davem, kys, netdev, Stephen Hemminger
In-Reply-To: <1485297559.16328.343.camel@edumazet-glaptop3.roam.corp.google.com>
On Tue, 24 Jan 2017 14:39:19 -0800
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2017-01-24 at 13:06 -0800, Stephen Hemminger wrote:
> > To improve performance, netvsc can call network stack directly and
> > avoid the local backlog queue. This is safe since incoming packets are
> > handled in softirq context already because the receive function
> > callback is called from a tasklet.
>
> Is this tasklet implementing a limit or something ?
The ring only holds a fixed amount of data so there is a limit but
it is quite large.
>
> netif_rx() queues packets to the backlog, which is processed later by
> net_rx_action() like other NAPI, with limit of 64 packets per round.
Since netvsc_receive has to copy all incoming data it is a bottleneck
unto itself. By the time net_rx_action is invoked the cache is stale.
>
> Calling netif_receive_skb() means you can escape this ability to fairly
> distribute the cpu cycles among multiple NAPI.
>
> I do not see range_cnt being capped in netvsc_receive()
There is no cap. NAPI is coming and will help.
^ permalink raw reply
* [PATCH net-next] tcp: reduce skb overhead in selected places
From: Eric Dumazet @ 2017-01-24 22:57 UTC (permalink / raw)
To: David Miller; +Cc: netdev
From: Eric Dumazet <edumazet@google.com>
tcp_add_backlog() can use skb_condense() helper to get better
gains and less SKB_TRUESIZE() magic. This only happens when socket
backlog has to be used.
Some attacks involve specially crafted out of order tiny TCP packets,
clogging the ofo queue of (many) sockets.
Then later, expensive collapse happens, trying to copy all these skbs
into single ones.
This unfortunately does not work if each skb has no neighbor in TCP
sequence order.
By using skb_condense() if the skb could not be coalesced to a prior
one, we defeat these kind of threats, potentially saving 4K per skb
(or more, since this is one page fragment).
A typical NAPI driver allocates gro packets with GRO_MAX_HEAD bytes
in skb->head, meaning the copy done by skb_condense() is limited to
about 200 bytes.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/ipv4/tcp_input.c | 1 +
net/ipv4/tcp_ipv4.c | 3 +--
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index bfa165cc455ad0a9aea44964aa663dbe6085..3de6eba378ade2c0d4a8400ecb5582a7d126 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4507,6 +4507,7 @@ static void tcp_data_queue_ofo(struct sock *sk, struct sk_buff *skb)
end:
if (skb) {
tcp_grow_window(sk, skb);
+ skb_condense(skb);
skb_set_owner_r(skb, sk);
}
}
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index f7325b25b06e65581ecc496f95e819aa738c..a90b4540c11eca6ed5b374ec69c8ced2ff18 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1556,8 +1556,7 @@ bool tcp_add_backlog(struct sock *sk, struct sk_buff *skb)
* It has been noticed pure SACK packets were sometimes dropped
* (if cooked by drivers without copybreak feature).
*/
- if (!skb->data_len)
- skb->truesize = SKB_TRUESIZE(skb_end_offset(skb));
+ skb_condense(skb);
if (unlikely(sk_add_backlog(sk, skb, limit))) {
bh_unlock_sock(sk);
^ permalink raw reply related
* Re: [PATCH net-next 00/18] netvsc driver enhancements for net-next
From: Stephen Hemminger @ 2017-01-24 22:50 UTC (permalink / raw)
To: David Miller; +Cc: kys, netdev, sthemmin
In-Reply-To: <20170124.163005.1454633748162819090.davem@davemloft.net>
On Tue, 24 Jan 2017 16:30:05 -0500 (EST)
David Miller <davem@davemloft.net> wrote:
> From: Stephen Hemminger <stephen@networkplumber.org>
> Date: Tue, 24 Jan 2017 13:05:57 -0800
>
> > Lots of little things in here. Support for minor more ethtool control,
> > negotiation of offload parameters with host (based on FreeBSD) and
> > several cleanups.
>
> Series applied, but two comments for potential follow-on changes:
There might be some merge conflicts in linux-next. Nothing drastic but
other vmbus changes are coming through that tree.
^ permalink raw reply
* Re: [PATCH 18/18] netvsc: call netif_receive_skb
From: Eric Dumazet @ 2017-01-24 22:39 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: davem, kys, netdev, Stephen Hemminger
In-Reply-To: <20170124210615.18628-19-sthemmin@microsoft.com>
On Tue, 2017-01-24 at 13:06 -0800, Stephen Hemminger wrote:
> To improve performance, netvsc can call network stack directly and
> avoid the local backlog queue. This is safe since incoming packets are
> handled in softirq context already because the receive function
> callback is called from a tasklet.
Is this tasklet implementing a limit or something ?
netif_rx() queues packets to the backlog, which is processed later by
net_rx_action() like other NAPI, with limit of 64 packets per round.
Calling netif_receive_skb() means you can escape this ability to fairly
distribute the cpu cycles among multiple NAPI.
I do not see range_cnt being capped in netvsc_receive()
^ permalink raw reply
* Re: [PATCH net] net: dsa: Keep a reference count on ethernet_dev
From: Florian Fainelli @ 2017-01-24 22:29 UTC (permalink / raw)
To: David Miller; +Cc: netdev, andrew, vivien.didelot, linux-kernel
In-Reply-To: <20170124.123911.330293119907498159.davem@davemloft.net>
On 01/24/2017 09:39 AM, David Miller wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Sat, 21 Jan 2017 09:40:54 -0800
>
>> of_find_net_device_by_node() just returns a reference to a net_device but does
>> not increment its reference count, which means that the master network device
>> can just vanish under our feet.
>>
>> Fixes: 83c0afaec7b7 ("net: dsa: Add new binding implementation")
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>
> This is fine, except now this netdev is completely locked into place with
> no way to dynamically unload it.
>
> If someone tries to modunload the driver for this ethernet device,
> their screen will fill up with warning messages indicating that the
> reference taken here in the DSA code is not going away.
>
> You need to implement a netdev notifier that tears down this DSA
> instance during an unregister event and releases the ethernet_dev.
> Similar to how we handle protocol addresses bound to a netdev, etc.
I have been thinking about this a bit more, and this is what is going on:
- upon master network device unregister we can look up the
dsa_switch_tree in dev->dsa_ptr and call dsa_dst_unapply() that is
actually exactly what we want since it detaches the dsa_switch_tree from
the master network device
- upon master network device register, we can look up whether this
master netdev is the one of interest and re-attach the dangling switch
tree, but here we have two cases:
- if we have an OF enabled system, doing a reverse look up of
net_device to device to device_node, and then looking it up in the
Device Tree is possible and reasonably simple, this works
- if we have a platform data enabled system, doing a reverse lookup is
possible, but won't necessarily yield the expected results, because
platform data will have a device reference to the original master
netdev, and this one could be totally different the second time we probe
the master network device due to to unregister/register
Thoughts?
--
Florian
^ permalink raw reply
* Re: [PATCH v2] bpf: Restrict cgroup bpf hooks to the init netns
From: Tejun Heo @ 2017-01-24 22:18 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Alexei Starovoitov, David Ahern, Andy Lutomirski,
Network Development, David S. Miller, Daniel Borkmann,
Alexei Starovoitov
In-Reply-To: <CALCETrW-Sj9eM+Nm9PVpqJky_30YSGhzc8hnzmddPkhHzb5_Fg@mail.gmail.com>
Hello, Andy.
On Tue, Jan 24, 2017 at 10:54:03AM -0800, Andy Lutomirski wrote:
> Tejun, I can see two basic ways that cgroup+bpf delegation could work
> down the road. Both depend on unprivileged users being able to load
> BPF programs of the correct type, but that's mostly a matter of
> auditing the code and doesn't have any particularly interesting design
> challenges as far as I know.
>
> Approach 1: Scope cgroup+bpf hooks to a netns and require
> ns_capable(CAP_NET_ADMIN), fs permissions, and (optionally) delegation
> to be enabled to install them. This shouldn't have any particularly
> dangerous security implications because ns_capable(CAP_NET_ADMIN)
> means you already own the netns.
>
> Approach 2: Keep cgroup+bpf hooks global. This makes the delegation
> story much trickier. There needs to be some mechanism to prevent some
> program that has delegated cgroup control from affecting the behavior
> of outside programs. This isn't so easy. Imagine that you've
> delegated /cgroup/foo to UID 1000. A program with UID 1000 can now
> install a hook on /cgroup/foo, but there needs to be a mechanism to
> prevent UID 1000 from running in /cgroup/foo in the global netns and
> then running a tool like sudo. This *might* be possible using just
> existing cgroup mechanisms, but it's going to be quite tricky to make
> sure it's done completely correctly and without dangerous races.
>
> If cgroup+bpf stays global, then I think you're basically committing
> to approach 2.
>
> I would suggest doing approach 1 (i.e. apply my patch) and then, if
> truly needed for some use case, add an option so that globally
> privileged programs can create hooks that affect all namespaces.
I thought about restricting according to namespaces and your ifindex
example. While the ifindex issue seems broken and on the surface
seems to indicate that we need to segregate cgroup bpf programs per
netns as iptables does, the more I think about it, the less convinced
I'm that that is the only right way forward.
I'm not too familiar with netns but if you think about other
namespaces, !root namespaces nest inside the root one. When you enter
a pid namespace, it adds its pid namespace on top of the global one.
It doesn't replace that, and that is an important characteristic which
allows the root namespace to maintain control over the nested ones.
This is the same with cgroupns, which basically is just nesting, or
mount namespace. Namespaced objects don't disappear in the eyes of
the root namespace. They just get additional scoped names.
Back to netns, if I'm understanding correctly, this doesn't seem to be
the case. An interface belongs to a namespace and can be moved around
but it disappears from the root namespace once it enters a non-root
one. I guess there are reasons, even if historical, for it but this
is what is breaking your ifindex example. It's fine for an interface
to get a new scoped ifindex inside a netns, but that shouldn't
override the global one. This, I think, should be fixable without
breaking existing APIs by introducing a new global index.
My opinion on this isn't very strong but what iptables is doing
actually seems wrong to me in that it actually precludes any way of
implementing global policies which encompasses the whole system.
Again, this is a fixable problem if ever necessary by introducing
properly global rule tables or even just adding a flag.
So, I don't think the situation is as clear as "ifindex is broken, so
cgroup bpf programs should be segregated per netns".
David, in another reply, you mentioned about fixing ifindex. If I
understood correctly, we're talking about the same thing, right? If
so, is this something easy to implement?
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH net-next 00/18] netvsc driver enhancements for net-next
From: Stephen Hemminger @ 2017-01-24 22:00 UTC (permalink / raw)
To: David Miller; +Cc: kys, netdev, sthemmin
In-Reply-To: <20170124.163005.1454633748162819090.davem@davemloft.net>
On Tue, 24 Jan 2017 16:30:05 -0500 (EST)
David Miller <davem@davemloft.net> wrote:
> From: Stephen Hemminger <stephen@networkplumber.org>
> Date: Tue, 24 Jan 2017 13:05:57 -0800
>
> > Lots of little things in here. Support for minor more ethtool control,
> > negotiation of offload parameters with host (based on FreeBSD) and
> > several cleanups.
>
> Series applied, but two comments for potential follow-on changes:
>
> > netvsc: enhance transmit select_queue
>
> Wouldn't it be better to check for the channel table NULL pointer
> before assigning the queue index to the socket?
Wanted to keep the original ordering out of paranoia.
> > netvsc: call netif_receive_skb
>
> Hmmm, potential stack depth issues?
No more of an issue than net softirq before (ie process_backlog).
The call chain is:
tasklet_action
vmbus_on_event ... process_chn_event
netvsc_channel_cb ... netif_receive_skb
NAPI/GRO support is possible but needs changes in VMBUS core code.
^ permalink raw reply
* Re: [PATCH v2] virtio_net: fix PAGE_SIZE > 64k
From: Michael S. Tsirkin @ 2017-01-24 21:56 UTC (permalink / raw)
To: David Miller; +Cc: netdev, john.fastabend, linux-kernel, virtualization
In-Reply-To: <20170124.161046.824400071485171478.davem@davemloft.net>
On Tue, Jan 24, 2017 at 04:10:46PM -0500, David Miller wrote:
> This works in the regimen that XDP packets always live in exactly one
> page. That will be needed to mmap the RX ring into userspace, and it
> helps make adjust_header trivial as well.
I think the point was to avoid resets across xdp attach/detach. If we
are doing resets now, we could do whatever buffering we want. We could
also just disable mergeable buffers for that matter.
> MTU 1500, PAGESIZE >= 4096, so a headroom of 256 is no problem, and
> we still have enough tailroom for skb_shared_info should we wrap
> the buffer into a real SKB and push it into the stack.
>
> If you are trying to do buffering differently for virtio_net, well...
> that's a self inflicted wound as far as I can tell.
Right but I was wondering about the fact that this makes XDP_PASS
much slower than processing skbs without XDP, as truesize is huge
so we'll quickly run out of rmem space.
When XDP is used to fight DOS attacks, why isn't this a concern?
--
MST
^ permalink raw reply
* Re: [PATCH v2] bpf: Restrict cgroup bpf hooks to the init netns
From: David Ahern @ 2017-01-24 21:34 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Alexei Starovoitov, Tejun Heo, Andy Lutomirski,
Network Development, David S. Miller, Daniel Borkmann,
Alexei Starovoitov
In-Reply-To: <CALCETrXzh-7BJ4Q_oG0hBkTdK+XgDWUYe0=gLwkqG6sncskv3g@mail.gmail.com>
On 1/24/17 2:24 PM, Andy Lutomirski wrote:
> I was hoping for an actual likely use case for the bpf hooks to be run
> in all namespaces. You're arguing that iproute2 can be made to work
> mostly okay if bpf hooks can run in all namespaces, but the use case
> of intentionally making sk_bound_dev_if invalid across all namespaces
> seems dubious.
you can use the bpf hook to deny socket create based on family and/or protocol.
^ permalink raw reply
* Re: [PATCH net-next 0/5] External MDIO support for mv88e6xxx
From: David Miller @ 2017-01-24 21:32 UTC (permalink / raw)
To: vivien.didelot; +Cc: andrew, netdev, f.fainelli
In-Reply-To: <87mvegxiag.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Tue, 24 Jan 2017 16:24:55 -0500
> David Miller <davem@davemloft.net> writes:
>
>> And maybe I should apply patches aggressively, as this lights a fire
>> under everyone's butt and people review and fix things more quickly.
>
> That has exactly the opposite effect. This makes people care less about
> each other work because "it's gonna be merged in a few minutes anyway."
Your very actions here disagree with this hypothesis. And, watch how
quickly Andrew will follow up.
:-)
^ permalink raw reply
* Re: [PATCH net-next 00/18] netvsc driver enhancements for net-next
From: David Miller @ 2017-01-24 21:30 UTC (permalink / raw)
To: stephen; +Cc: kys, netdev, sthemmin
In-Reply-To: <20170124210615.18628-1-sthemmin@microsoft.com>
From: Stephen Hemminger <stephen@networkplumber.org>
Date: Tue, 24 Jan 2017 13:05:57 -0800
> Lots of little things in here. Support for minor more ethtool control,
> negotiation of offload parameters with host (based on FreeBSD) and
> several cleanups.
Series applied, but two comments for potential follow-on changes:
> netvsc: enhance transmit select_queue
Wouldn't it be better to check for the channel table NULL pointer
before assigning the queue index to the socket?
> netvsc: call netif_receive_skb
Hmmm, potential stack depth issues?
^ permalink raw reply
* Re: [PATCH] ibmveth: Add a proper check for the availability of the checksum features
From: Thomas Falcon @ 2017-01-24 21:28 UTC (permalink / raw)
To: Thomas Huth, netdev; +Cc: linuxppc-dev
In-Reply-To: <1485239321-14947-1-git-send-email-thuth@redhat.com>
On 01/24/2017 12:28 AM, Thomas Huth wrote:
> When using the ibmveth driver in a KVM/QEMU based VM, it currently
> always prints out a scary error message like this when it is started:
>
> ibmveth 71000003 (unregistered net_device): unable to change
> checksum offload settings. 1 rc=-2 ret_attr=71000003
>
> This happens because the driver always tries to enable the checksum
> offloading without checking for the availability of this feature first.
> QEMU does not support checksum offloading for the spapr-vlan device,
> thus we always get the error message here.
> According to the LoPAPR specification, the "ibm,illan-options" property
> of the corresponding device tree node should be checked first to see
> whether the H_ILLAN_ATTRIUBTES hypercall and thus the checksum offloading
> feature is available. Thus let's do this in the ibmveth driver, too, so
> that the error message is really only limited to cases where something
> goes wrong, and does not occur if the feature is just missing.
Thanks a lot for this patch, Thomas. Was going to give an Ack, but its already been applied :)
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> drivers/net/ethernet/ibm/ibmveth.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
> index a831f94..309f5c6 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> @@ -1601,8 +1601,11 @@ static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
> netdev->netdev_ops = &ibmveth_netdev_ops;
> netdev->ethtool_ops = &netdev_ethtool_ops;
> SET_NETDEV_DEV(netdev, &dev->dev);
> - netdev->hw_features = NETIF_F_SG | NETIF_F_RXCSUM |
> - NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
> + netdev->hw_features = NETIF_F_SG;
> + if (vio_get_attribute(dev, "ibm,illan-options", NULL) != NULL) {
> + netdev->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> + NETIF_F_RXCSUM;
> + }
>
> netdev->features |= netdev->hw_features;
>
^ permalink raw reply
* Re: [PATCH net-next 0/5] External MDIO support for mv88e6xxx
From: Vivien Didelot @ 2017-01-24 21:24 UTC (permalink / raw)
To: David Miller; +Cc: andrew, netdev, f.fainelli
In-Reply-To: <20170124.160750.354090492587505672.davem@davemloft.net>
Hi David,
David Miller <davem@davemloft.net> writes:
>> Andrew just replied to my comment an hour ago, and I still have concerns
>> about the patchset. I didn't like the list_head of busses, and the 6352
>> family don't support the external mode. That needed to be addressed.
>>
>> Why such an hurry to merge patches?
>
> Because my backlog sucks otherwise?
>
> I can revert, or you can ask Andrew to post follow-on fixups to
> satisfy your concerns. Just because I applied it doesn't make it
> the end of the world.
>
> And maybe I should apply patches aggressively, as this lights a fire
> under everyone's butt and people review and fix things more quickly.
That has exactly the opposite effect. This makes people care less about
each other work because "it's gonna be merged in a few minutes anyway."
:-)
^ permalink raw reply
* Re: [PATCH v2] bpf: Restrict cgroup bpf hooks to the init netns
From: Andy Lutomirski @ 2017-01-24 21:24 UTC (permalink / raw)
To: David Ahern
Cc: Alexei Starovoitov, Tejun Heo, Andy Lutomirski,
Network Development, David S. Miller, Daniel Borkmann,
Alexei Starovoitov
In-Reply-To: <242993d5-e27c-7177-a021-610d16b150e4@cumulusnetworks.com>
On Tue, Jan 24, 2017 at 12:29 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
>
> Users do not run around exec'ing commands in random network contexts (namespace, vrf, device, whatever) and expect them to just work.
I worked on some code once (deployed in production, even!) that calls
unshare() to make a netns and creates an interface and runs code in
there and expects it to just work. It wouldn't work the outer program
were run under current ip vrf.
>
>>
>> Maybe you can argue that this is a missing feature in cgroup+bpf (no
>> API to query which netns is in use) and a corresponding bug in 'ip
>> vrf', but I see this as evidence that cgroup+bpf as it exists in 4.10
>> is not carefully enough throught through. The only non-example user
>> of it that I can find (ip vrf) is buggy and can't really be fixed
>> using mechanisms that exist in 4.10-rc.
>
> The argument is that cgroups and namespaces are completely disjoint infrastructure and that users need to know what they are doing.
But perhaps they should be less disjoint. As far as I know,
cgroup+bpf is the *only* network configuration that is being set up to
run across all network namespaces. No one has said why this is okay,
let alone why it's preferable to making it work per-netns just like
basically all other Linux network configuration.
>
>>
>>>
>>>> things up so that unshare will malfunction. It should avoid
>>>> malfunctioning when running Linux programs that are unaware of it.
>>>
>>> I agree that for VRF use case it will help to make programs netns
>>> aware by adding new bpf_get_current_netns_id() or something helper,
>>> but it's up to the program to function properly or be broken.
>>
>> This will cause David's code to run slower, and I think he wants very
>> high performance.
>
> This is a socket create path not a packet path. While overhead should be contained, a few extra cycles should be fine.
>
> Adding the capability to allow users to check the netns id would offer a solution to the namespace problem, but there is nothing that *requires* a bpf program to do it.
>
> Who's to say an admin does not *want* all processes in a group to have sockets bound to a non-existent device? 'ip vrf' restricts the device index to a VRF device because as a management tool I want it to be user friendly, but generically the BPF code does not have any restrictions. ifindex can be any u32 value.
I was hoping for an actual likely use case for the bpf hooks to be run
in all namespaces. You're arguing that iproute2 can be made to work
mostly okay if bpf hooks can run in all namespaces, but the use case
of intentionally making sk_bound_dev_if invalid across all namespaces
seems dubious.
But all of what you're suggesting doing would still work fine and
would result in less kernel code *and* less eBPF code if the hooks
were per netns.
--Andy
^ permalink raw reply
* Re: [PATCH net v3 0/2] net: Fix oops on state free after lwt module unload
From: David Miller @ 2017-01-24 21:21 UTC (permalink / raw)
To: rshearma; +Cc: netdev, tom, roopa
In-Reply-To: <1485275208-31702-1-git-send-email-rshearma@brocade.com>
From: Robert Shearman <rshearma@brocade.com>
Date: Tue, 24 Jan 2017 16:26:46 +0000
> An oops is seen in lwtstate_free after an lwt ops module has been
> unloaded. This patchset fixes this by preventing modules implementing
> lwtunnel ops from being unloaded whilst there's state alive using
> those ops. The first patch adds fills in a new owner field in all lwt
> ops and the second patch makes use of this to reference count the
> modules as state is built and destroyed using them.
>
> Changes in v3:
> - don't put module reference if try_module_get fails on building state
>
> Changes in v2:
> - specify module owner for all modules as suggested by DaveM
> - reference count all modules building lwt state, not just those ops
> implementing destroy_state, as also suggested by DaveM.
> - rebased on top of David Ahern's lwtunnel changes
Applied and queued up for -stable.
Thanks.
^ permalink raw reply
* Re: [PATCH v5 2/3] Bluetooth: btusb: Add out-of-band wakeup support
From: Brian Norris @ 2017-01-24 21:20 UTC (permalink / raw)
To: Rajat Jain
Cc: Rob Herring, Mark Rutland, Marcel Holtmann, Gustavo Padovan,
Johan Hedberg, Amitkumar Karwar, Wei-Ning Huang, Xinming Hu,
netdev, devicetree, linux-bluetooth, linux-kernel, rajatxjain
In-Reply-To: <20170112180107.63244-2-rajatja@google.com>
Hi Rajat,
On Thu, Jan 12, 2017 at 10:01:06AM -0800, Rajat Jain wrote:
> Some onboard BT chips (e.g. Marvell 8997) contain a wakeup pin that
> can be connected to a gpio on the CPU side, and can be used to wakeup
> the host out-of-band. This can be useful in situations where the
> in-band wakeup is not possible or not preferable (e.g. the in-band
> wakeup may require the USB host controller to remain active, and
> hence consuming more system power during system sleep).
>
> The oob gpio interrupt to be used for wakeup on the CPU side, is
> read from the device tree node, (using standard interrupt descriptors).
> A devcie tree binding document is also added for the driver. The
> compatible string is in compliance with
> Documentation/devicetree/bindings/usb/usb-device.txt
>
> Signed-off-by: Rajat Jain <rajatja@google.com>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
> v5: Move the call to pm_wakeup_event() to the begining of irq handler.
> v4: Move the set_bit(BTUSB_OOB_WAKE_DISABLED,..) call to the beginning of
> btusb_config_oob_wake()
> v3: Add Brian's "Reviewed-by"
> v2: * Use interrupt-names ("wakeup") instead of assuming first interrupt.
> * Leave it on device tree to specify IRQ flags (level /edge triggered)
> * Mark the device as non wakeable on exit.
>
> Documentation/devicetree/bindings/net/btusb.txt | 40 ++++++++++++
> drivers/bluetooth/btusb.c | 85 +++++++++++++++++++++++++
> 2 files changed, 125 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/btusb.txt
>
> diff --git a/Documentation/devicetree/bindings/net/btusb.txt b/Documentation/devicetree/bindings/net/btusb.txt
> new file mode 100644
> index 000000000000..2c0355c85972
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/btusb.txt
> @@ -0,0 +1,40 @@
> +Generic Bluetooth controller over USB (btusb driver)
> +---------------------------------------------------
> +
> +Required properties:
> +
> + - compatible : should comply with the format "usbVID,PID" specified in
> + Documentation/devicetree/bindings/usb/usb-device.txt
> + At the time of writing, the only OF supported devices
> + (more may be added later) are:
> +
> + "usb1286,204e" (Marvell 8997)
> +
> +Optional properties:
> +
> + - interrupt-parent: phandle of the parent interrupt controller
> + - interrupt-names: (see below)
> + - interrupts : The interrupt specified by the name "wakeup" is the interrupt
> + that shall be used for out-of-band wake-on-bt. Driver will
> + request this interrupt for wakeup. During system suspend, the
> + irq will be enabled so that the bluetooth chip can wakeup host
> + platform out of band. During system resume, the irq will be
> + disabled to make sure unnecessary interrupt is not received.
> +
> +Example:
> +
> +Following example uses irq pin number 3 of gpio0 for out of band wake-on-bt:
> +
> +&usb_host1_ehci {
> + status = "okay";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + mvl_bt1: bt@1 {
> + compatible = "usb1286,204e";
> + reg = <1>;
> + interrupt-parent = <&gpio0>;
> + interrupt-name = "wakeup";
> + interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
> + };
> +};
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index ce22cefceed1..0a777bb407b1 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -24,6 +24,8 @@
> #include <linux/module.h>
> #include <linux/usb.h>
> #include <linux/firmware.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> #include <asm/unaligned.h>
>
> #include <net/bluetooth/bluetooth.h>
> @@ -369,6 +371,7 @@ static const struct usb_device_id blacklist_table[] = {
> #define BTUSB_BOOTING 9
> #define BTUSB_RESET_RESUME 10
> #define BTUSB_DIAG_RUNNING 11
> +#define BTUSB_OOB_WAKE_DISABLED 12
>
> struct btusb_data {
> struct hci_dev *hdev;
> @@ -416,6 +419,8 @@ struct btusb_data {
> int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
>
> int (*setup_on_usb)(struct hci_dev *hdev);
> +
> + int oob_wake_irq; /* irq for out-of-band wake-on-bt */
> };
>
> static inline void btusb_free_frags(struct btusb_data *data)
> @@ -2728,6 +2733,66 @@ static int btusb_bcm_set_diag(struct hci_dev *hdev, bool enable)
> }
> #endif
>
> +#ifdef CONFIG_PM
> +static irqreturn_t btusb_oob_wake_handler(int irq, void *priv)
> +{
> + struct btusb_data *data = priv;
> +
> + pm_wakeup_event(&data->udev->dev, 0);
> +
> + /* Disable only if not already disabled (keep it balanced) */
> + if (!test_and_set_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags)) {
> + disable_irq_nosync(irq);
> + disable_irq_wake(irq);
> + }
> + return IRQ_HANDLED;
> +}
> +
> +static const struct of_device_id btusb_match_table[] = {
> + { .compatible = "usb1286,204e" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, btusb_match_table);
> +
> +/* Use an oob wakeup pin? */
> +static int btusb_config_oob_wake(struct hci_dev *hdev)
> +{
> + struct btusb_data *data = hci_get_drvdata(hdev);
> + struct device *dev = &data->udev->dev;
> + int irq, ret;
> +
> + set_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags);
> +
> + if (!of_match_device(btusb_match_table, dev))
> + return 0;
> +
> + /* Move on if no IRQ specified */
> + irq = of_irq_get_byname(dev->of_node, "wakeup");
> + if (irq <= 0) {
> + bt_dev_dbg(hdev, "%s: no OOB Wakeup IRQ in DT", __func__);
> + return 0;
> + }
> +
> + ret = devm_request_irq(&hdev->dev, irq, btusb_oob_wake_handler,
> + 0, "OOB Wake-on-BT", data);
> + if (ret) {
> + bt_dev_err(hdev, "%s: IRQ request failed", __func__);
> + return ret;
> + }
> +
> + ret = device_init_wakeup(dev, true);
> + if (ret) {
> + bt_dev_err(hdev, "%s: failed to init_wakeup\n", __func__);
bt_dev_err() includes the newlines for you, but you'd added an extra one
here (but not above).
> + return ret;
> + }
> +
> + data->oob_wake_irq = irq;
> + disable_irq(irq);
> + bt_dev_info(hdev, "OOB Wake-on-BT configured at IRQ %u\n", irq);
And here.
> + return 0;
> +}
> +#endif
> +
> static int btusb_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> {
> @@ -2849,6 +2914,11 @@ static int btusb_probe(struct usb_interface *intf,
> hdev->send = btusb_send_frame;
> hdev->notify = btusb_notify;
>
> +#ifdef CONFIG_PM
> + err = btusb_config_oob_wake(hdev);
> + if (err)
> + goto out_free_dev;
> +#endif
> if (id->driver_info & BTUSB_CW6622)
> set_bit(HCI_QUIRK_BROKEN_STORED_LINK_KEY, &hdev->quirks);
>
> @@ -3061,6 +3131,9 @@ static void btusb_disconnect(struct usb_interface *intf)
> usb_driver_release_interface(&btusb_driver, data->isoc);
> }
>
> + if (data->oob_wake_irq)
> + device_init_wakeup(&data->udev->dev, false);
> +
> hci_free_dev(hdev);
> }
>
> @@ -3089,6 +3162,12 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
> btusb_stop_traffic(data);
> usb_kill_anchored_urbs(&data->tx_anchor);
>
> + if (data->oob_wake_irq && device_may_wakeup(&data->udev->dev)) {
> + clear_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags);
> + enable_irq_wake(data->oob_wake_irq);
> + enable_irq(data->oob_wake_irq);
> + }
> +
> /* Optionally request a device reset on resume, but only when
> * wakeups are disabled. If wakeups are enabled we assume the
> * device will stay powered up throughout suspend.
> @@ -3126,6 +3205,12 @@ static int btusb_resume(struct usb_interface *intf)
> if (--data->suspend_count)
> return 0;
>
> + /* Disable only if not already disabled (keep it balanced) */
> + if (!test_and_set_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags)) {
As I mentioned elsewhere, the negative form (i.e., DISABLED instead of
ENABLED) seems a little backward to me. It has the small effect of
meaning that the default behavior is actually to pretend that OOB wake
was enabled, and so if somehow btusb_config_oob_wake() wasn't called
(e.g., if CONFIG_PM is not enabled, or if the code gets refactored) this
then btusb_resume() will behave badly.
Now, this doesn't create an immediate problem (btusb_resume() should
never be called if !CONFIG_PM), but it does suggest that maybe it would
be better for the default (0) value to mean "disabled".
Brian
> + disable_irq(data->oob_wake_irq);
> + disable_irq_wake(data->oob_wake_irq);
> + }
> +
> if (!test_bit(HCI_RUNNING, &hdev->flags))
> goto done;
>
> --
> 2.11.0.390.gc69c2f50cf-goog
>
^ permalink raw reply
* Re: [PATCH net] net: free ip_vs_dest structs when refcnt=0
From: David Miller @ 2017-01-24 21:17 UTC (permalink / raw)
To: ja; +Cc: dwindsor, netdev, kernel-hardening, keescook, elena.reshetova,
ishkamiel
In-Reply-To: <alpine.LFD.2.11.1701242255570.2743@ja.home.ssi.bg>
From: Julian Anastasov <ja@ssi.bg>
Date: Tue, 24 Jan 2017 23:14:29 +0200 (EET)
> The changes look ok to me but as this is a net-next
> material I also prefer some comments in the code to be updated:
It also needs to be posted to netfilter-devel with the IPVS maintainer
CC:'d
^ permalink raw reply
* Re: [PATCH net v1 0/6] topology server fixes for nametable soft lockup
From: David Miller @ 2017-01-24 21:16 UTC (permalink / raw)
To: parthasarathy.bhuvaragan; +Cc: netdev, tipc-discussion, jon.maloy, ying.xue
In-Reply-To: <1485259248-21812-1-git-send-email-parthasarathy.bhuvaragan@ericsson.com>
From: Parthasarathy Bhuvaragan <parthasarathy.bhuvaragan@ericsson.com>
Date: Tue, 24 Jan 2017 13:00:42 +0100
> In this series, we revert the commit 333f796235a527 ("tipc: fix a
> race condition leading to subscriber refcnt bug") and provide an
> alternate solution to fix the race conditions in commits 2-4.
>
> We have to do this as the above commit introduced a nametbl soft
> lockup at module exit as described by patch#4.
Series applied, thanks.
^ permalink raw reply
* [PATCH net 2/3] qed: Read queue state before releasing buffer
From: Yuval Mintz @ 2017-01-24 21:15 UTC (permalink / raw)
To: davem, netdev; +Cc: linux-rdma, Ram.Amrani, Yuval Mintz
In-Reply-To: <1485292523-8821-1-git-send-email-Yuval.Mintz@cavium.com>
From: Ram Amrani <Ram.Amrani@cavium.com>
Currently the state is read only after the buffers are relesed.
Signed-off-by: Ram Amrani <Ram.Amrani@cavium.com>
Signed-off-by: Yuval Mintz <Yuval.Mintz@cavium.com>
---
drivers/net/ethernet/qlogic/qed/qed_roce.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_roce.c b/drivers/net/ethernet/qlogic/qed/qed_roce.c
index 7ab6d4e..0846068 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_roce.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_roce.c
@@ -1768,13 +1768,13 @@ static int qed_roce_query_qp(struct qed_hwfn *p_hwfn,
if (rc)
goto err_resp;
- dma_free_coherent(&p_hwfn->cdev->pdev->dev, sizeof(*p_resp_ramrod_res),
- p_resp_ramrod_res, resp_ramrod_res_phys);
-
out_params->rq_psn = le32_to_cpu(p_resp_ramrod_res->psn);
rq_err_state = GET_FIELD(le32_to_cpu(p_resp_ramrod_res->err_flag),
ROCE_QUERY_QP_RESP_OUTPUT_PARAMS_ERROR_FLG);
+ dma_free_coherent(&p_hwfn->cdev->pdev->dev, sizeof(*p_resp_ramrod_res),
+ p_resp_ramrod_res, resp_ramrod_res_phys);
+
if (!(qp->req_offloaded)) {
/* Don't send query qp for the requester */
out_params->sq_psn = qp->sq_psn;
@@ -1815,9 +1815,6 @@ static int qed_roce_query_qp(struct qed_hwfn *p_hwfn,
if (rc)
goto err_req;
- dma_free_coherent(&p_hwfn->cdev->pdev->dev, sizeof(*p_req_ramrod_res),
- p_req_ramrod_res, req_ramrod_res_phys);
-
out_params->sq_psn = le32_to_cpu(p_req_ramrod_res->psn);
sq_err_state = GET_FIELD(le32_to_cpu(p_req_ramrod_res->flags),
ROCE_QUERY_QP_REQ_OUTPUT_PARAMS_ERR_FLG);
@@ -1825,6 +1822,9 @@ static int qed_roce_query_qp(struct qed_hwfn *p_hwfn,
GET_FIELD(le32_to_cpu(p_req_ramrod_res->flags),
ROCE_QUERY_QP_REQ_OUTPUT_PARAMS_SQ_DRAINING_FLG);
+ dma_free_coherent(&p_hwfn->cdev->pdev->dev, sizeof(*p_req_ramrod_res),
+ p_req_ramrod_res, req_ramrod_res_phys);
+
out_params->draining = false;
if (rq_err_state)
--
1.8.3.1
^ permalink raw reply related
* [PATCH net 1/3] qed: Release CQ resource under lock on failure
From: Yuval Mintz @ 2017-01-24 21:15 UTC (permalink / raw)
To: davem, netdev; +Cc: linux-rdma, Ram.Amrani, Yuval Mintz
In-Reply-To: <1485292523-8821-1-git-send-email-Yuval.Mintz@cavium.com>
From: Ram Amrani <Ram.Amrani@cavium.com>
The CQ resource pool is protected by a spin lock. When a CQ creation
fails it now deallocates under that lock as well.
Signed-off-by: Ram Amrani <Ram.Amrani@cavium.com>
Signed-off-by: Yuval Mintz <Yuval.Mintz@cavium.com>
---
drivers/net/ethernet/qlogic/qed/qed_roce.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_roce.c b/drivers/net/ethernet/qlogic/qed/qed_roce.c
index bd4cad2..7ab6d4e 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_roce.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_roce.c
@@ -948,7 +948,9 @@ static int qed_rdma_create_cq(void *rdma_cxt,
err:
/* release allocated icid */
+ spin_lock_bh(&p_info->lock);
qed_bmap_release_id(p_hwfn, &p_info->cq_map, returned_id);
+ spin_unlock_bh(&p_info->lock);
DP_NOTICE(p_hwfn, "Create CQ failed, rc = %d\n", rc);
return rc;
--
1.8.3.1
^ permalink raw reply related
* [PATCH net 3/3] qed: Don't free a QP more than once
From: Yuval Mintz @ 2017-01-24 21:15 UTC (permalink / raw)
To: davem-fT/PcQaiUtIeIZ0/mPfg9Q, netdev-u79uwXL29TY76Z2rM5mHXA
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA,
Ram.Amrani-YGCgFSpz5w/QT0dZR+AlfA, Yuval Mintz
In-Reply-To: <1485292523-8821-1-git-send-email-Yuval.Mintz-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
From: Ram Amrani <Ram.Amrani-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
If QP is in reset state then there are no resources to free so avoid
freeing any.
Signed-off-by: Ram Amrani <Ram.Amrani-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Yuval Mintz <Yuval.Mintz-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
---
drivers/net/ethernet/qlogic/qed/qed_roce.c | 49 ++++++++++++++++--------------
1 file changed, 27 insertions(+), 22 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_roce.c b/drivers/net/ethernet/qlogic/qed/qed_roce.c
index 0846068..a074c76 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_roce.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_roce.c
@@ -1849,6 +1849,7 @@ static int qed_roce_query_qp(struct qed_hwfn *p_hwfn,
static int qed_roce_destroy_qp(struct qed_hwfn *p_hwfn, struct qed_rdma_qp *qp)
{
+ struct qed_rdma_info *p_rdma_info = p_hwfn->p_rdma_info;
u32 num_invalidated_mw = 0;
u32 num_bound_mw = 0;
u32 start_cid;
@@ -1863,35 +1864,39 @@ static int qed_roce_destroy_qp(struct qed_hwfn *p_hwfn, struct qed_rdma_qp *qp)
return -EINVAL;
}
- rc = qed_roce_sp_destroy_qp_responder(p_hwfn, qp, &num_invalidated_mw);
- if (rc)
- return rc;
+ if (qp->cur_state != QED_ROCE_QP_STATE_RESET) {
+ rc = qed_roce_sp_destroy_qp_responder(p_hwfn, qp,
+ &num_invalidated_mw);
+ if (rc)
+ return rc;
- /* Send destroy requester ramrod */
- rc = qed_roce_sp_destroy_qp_requester(p_hwfn, qp, &num_bound_mw);
- if (rc)
- return rc;
+ /* Send destroy requester ramrod */
+ rc = qed_roce_sp_destroy_qp_requester(p_hwfn, qp,
+ &num_bound_mw);
+ if (rc)
+ return rc;
- if (num_invalidated_mw != num_bound_mw) {
- DP_NOTICE(p_hwfn,
- "number of invalidate memory windows is different from bounded ones\n");
- return -EINVAL;
- }
+ if (num_invalidated_mw != num_bound_mw) {
+ DP_NOTICE(p_hwfn,
+ "number of invalidate memory windows is different from bounded ones\n");
+ return -EINVAL;
+ }
- spin_lock_bh(&p_hwfn->p_rdma_info->lock);
+ spin_lock_bh(&p_rdma_info->lock);
- start_cid = qed_cxt_get_proto_cid_start(p_hwfn,
- p_hwfn->p_rdma_info->proto);
+ start_cid = qed_cxt_get_proto_cid_start(p_hwfn,
+ p_rdma_info->proto);
- /* Release responder's icid */
- qed_bmap_release_id(p_hwfn, &p_hwfn->p_rdma_info->cid_map,
- qp->icid - start_cid);
+ /* Release responder's icid */
+ qed_bmap_release_id(p_hwfn, &p_rdma_info->cid_map,
+ qp->icid - start_cid);
- /* Release requester's icid */
- qed_bmap_release_id(p_hwfn, &p_hwfn->p_rdma_info->cid_map,
- qp->icid + 1 - start_cid);
+ /* Release requester's icid */
+ qed_bmap_release_id(p_hwfn, &p_rdma_info->cid_map,
+ qp->icid + 1 - start_cid);
- spin_unlock_bh(&p_hwfn->p_rdma_info->lock);
+ spin_unlock_bh(&p_rdma_info->lock);
+ }
return 0;
}
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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 0/3] qed: RoCE infrastrucutre fixes
From: Yuval Mintz @ 2017-01-24 21:15 UTC (permalink / raw)
To: davem-fT/PcQaiUtIeIZ0/mPfg9Q, netdev-u79uwXL29TY76Z2rM5mHXA
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA,
Ram.Amrani-YGCgFSpz5w/QT0dZR+AlfA, Yuval Mintz
This series contains three RoCE bug fixes that improve stability and
robustness.
Dave,
Please consider applying these to 'net'.
Thanks,
Yuval
Ram Amrani (3):
qed: on failure release CQ resource under lock
qed: read queue state before releasing buffer
qed: don't free a QP more than once
drivers/net/ethernet/qlogic/qed/qed_roce.c | 63 +++++++++++++++++-------------
1 file changed, 35 insertions(+), 28 deletions(-)
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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 net] net: free ip_vs_dest structs when refcnt=0
From: Julian Anastasov @ 2017-01-24 21:14 UTC (permalink / raw)
To: David Windsor
Cc: netdev, kernel-hardening, keescook, elena.reshetova, ishkamiel
In-Reply-To: <1485262100-12366-1-git-send-email-dwindsor@gmail.com>
Hello,
On Tue, 24 Jan 2017, David Windsor wrote:
> Currently, the ip_vs_dest cache frees ip_vs_dest objects when their reference
> count becomes < 0. Aside from not being semantically sound, this is problematic
> for the new type refcount_t, which will be introduced shortly in a separate patch.
> refcount_t is the new kernel type for holding reference counts, and provides
> overflow protection and a constrained interface relative to atomic_t (the type
> currently being used for kernel reference counts).
>
> Per Juilan Anastasov: "The problem is that dest_trash currently holds deleted
Julian
> dests (unlinked from RCU lists) with refcnt=0." Changing dest_trash to hold
> dest with refcnt=1 will allow us to free ip_vs_dest structs when their refcnt=0,
> in ip_vs_dest_put_and_free().
Too long lines, should be up to 75 columns as per
Documentation/process/submitting-patches.rst
Check with: scripts/checkpatch.pl --strict /tmp/file.patch
> Signed-off-by: David Windsor <dwindsor@gmail.com>
> ---
> include/net/ip_vs.h | 2 +-
> net/netfilter/ipvs/ip_vs_ctl.c | 4 +---
> 2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
> index 55e0169..6b5492e 100644
> --- a/net/netfilter/ipvs/ip_vs_ctl.c
> +++ b/net/netfilter/ipvs/ip_vs_ctl.c
> @@ -711,7 +711,6 @@ ip_vs_trash_get_dest(struct ip_vs_service *svc, int dest_af,
> dest->vport == svc->port))) {
> /* HIT */
> list_del(&dest->t_list);
> - ip_vs_dest_hold(dest);
> goto out;
> }
> }
The changes look ok to me but as this is a net-next
material I also prefer some comments in the code to be updated:
- "be 0, so we simply release them here" before ip_vs_trash_cleanup
should become "be 1, so we simply release them here".
- "dest lives in trash with[out] reference" in __ip_vs_del_dest
When sending v2 for net-next please also CC:
Simon Horman <horms@verge.net.au>
Pablo Neira Ayuso <pablo@netfilter.org>
lvs-devel@vger.kernel.org
I'll ack the next version after some tests...
Regards
^ permalink raw reply
* Re: [PATCH net-next 0/5] External MDIO support for mv88e6xxx
From: Vivien Didelot @ 2017-01-24 21:05 UTC (permalink / raw)
To: David Miller, andrew; +Cc: netdev, f.fainelli
In-Reply-To: <20170124.153504.1132892597644436492.davem@davemloft.net>
Hi David,
David Miller <davem@davemloft.net> writes:
> Series applied, thanks Andrew.
>
> There seems to be a bit of inconsistency wrt. tab vs. space for
> indentation in the binding files, just FYI. I had to make some small
> adjustments to this patch series in order to keep GIT from warning
> when I applied this stuff.
Andrew just replied to my comment an hour ago, and I still have concerns
about the patchset. I didn't like the list_head of busses, and the 6352
family don't support the external mode. That needed to be addressed.
Why such an hurry to merge patches?
Vivien
^ 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