* Re: [PATCH v2 7/7] driver-core: add preferred async probe option for built-in and modules
From: Tejun Heo @ 2014-10-06 21:01 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Luis R. Rodriguez, gregkh, dmitry.torokhov, tiwai, arjan, teg,
rmilasan, werner, oleg, hare, bpoirier, santosh, pmladek, dbueso,
linux-kernel, Tetsuo Handa, Joseph Salisbury, Kay Sievers,
One Thousand Gnomes, Tim Gardner, Pierre Fersing, Andrew Morton,
Nagalakshmi Nandigama, Praveen Krishnamoorthy, Sreekanth Reddy,
Abhijit Mahajan, Casey Leedom
In-Reply-To: <20141006203627.GZ14081@wotan.suse.de>
Hello,
On Mon, Oct 06, 2014 at 10:36:27PM +0200, Luis R. Rodriguez wrote:
> > Do we intend to keep this param permanently? Isn't this more of a
> > temp tool to be used during development? If so, maybe we should make
> > that clear with __DEVEL__ too?
>
> As its designed right now no, its not a temp tool, its there to
> require compatibility with old userspace. For modules we can require
> the module parameter but for built-in we need something else and this
> is what came to mind. It is also what would allow the prefer_async_probe
> flag too as otherwise we won't know if userspace is prepared.
I don't get it. For in-kernel stuff, we already have a clear
synchronization point where we already synchronize all async calls.
Shouldn't we be flushing these async probes there too? insmod'ing is
userland visible but there's no reason this has to be for the built-in
drivers.
Thanks.
--
tejun
^ permalink raw reply
* Re: [Xen-devel] [PATCHv1] xen-netfront: always keep the Rx ring full of requests
From: David Miller @ 2014-10-06 21:07 UTC (permalink / raw)
To: annie.li; +Cc: david.vrabel, netdev, boris.ostrovsky, xen-devel
In-Reply-To: <5432E26C.3020207@oracle.com>
From: annie li <annie.li@oracle.com>
Date: Mon, 06 Oct 2014 14:41:48 -0400
>
> On 2014/10/6 12:00, David Vrabel wrote:
>>>> + queue->rx.req_prod_pvt = req_prod;
>>>> +
>>>> + /* Not enough requests? Try again later. */
>>>> + if (req_prod - queue->rx.rsp_cons < NET_RX_SLOTS_MIN) {
>>>> + mod_timer(&queue->rx_refill_timer, jiffies + (HZ/10));
>>>> + return;
>>> If the previous for loop breaks because of failure of
>>> xennet_alloc_one_rx_buffer, then notify_remote_via_irq is missed here
>>> if
>>> the code returns directly.
>> This is deliberate -- there's no point notifying the backend if there
>> aren't enough requests for the next packet. Since we don't know what
>> the next packet might be we assume it's the largest possible.
> That makes sense.
> However, the largest packet case does not happen so
> frequently. Moreover, netback checks the slots every incoming skb
> requires in xenvif_rx_ring_slots_available, not only concerning the
> largest case.
I have an opinion about the sysfs stuff.
It's user facing, so even if it doesn't influence behavior any more
you have to keep the files around, just make them nops.
^ permalink raw reply
* Re: [PATCH RFC net 0/2] ipv6: Avoid restarting fib6_lookup() for RTF_CACHE hit
From: David Miller @ 2014-10-06 21:10 UTC (permalink / raw)
To: kafai; +Cc: netdev, hannes
In-Reply-To: <1412374332-26657-1-git-send-email-kafai@fb.com>
From: Martin KaFai Lau <kafai@fb.com>
Date: Fri, 3 Oct 2014 15:12:10 -0700
> I am trying to understand why there is a need to restart fib6_lookup() after
> getting rt with RTF_CACHE.
>
> I have adapted the davem's udpflood test
> (https://git.kernel.org/pub/scm/linux/kernel/git/davem/net_test_tools.git) to
> support IPv6 and here is the result:
>
> #root > time ./udpflood -l 20000000 -c 250 2401:db00:face:face::2
>
> Before:
> real 0m33.224s
> user 0m2.941s
> sys 0m30.232s
>
> After:
> real 0m31.517s
> user 0m2.938s
> sys 0m28.536s
If you are serious about seeing these patches integrated, you must
freshly repost this series and provide a proper "Signed-off-by: " tag
for yourself in the commit messages.
^ permalink raw reply
* Re: [PATCH v8 net-next 1/2] bonding: display xmit_hash_policy for non-dynamic-tlb mode
From: David Miller @ 2014-10-06 21:13 UTC (permalink / raw)
To: maheshb; +Cc: j.vosburgh, andy, vfalico, nikolay, netdev, edumazet, maze
In-Reply-To: <1412469884-27308-1-git-send-email-maheshb@google.com>
From: Mahesh Bandewar <maheshb@google.com>
Date: Sat, 4 Oct 2014 17:44:44 -0700
> It's a trivial fix to display xmit_hash_policy for this new TLB mode
> since it uses transmit-hash-poilicy as part of bonding-master info
> (/proc/net/bonding/<bonding-interface).
>
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> Reviewed-by: Nikolay Aleksandrov <nikolay@redhat.com>
Applied.
^ permalink raw reply
* Re: [PATCH v8 net-next 2/2] bonding: Simplify the xmit function for modes that use xmit_hash
From: David Miller @ 2014-10-06 21:13 UTC (permalink / raw)
To: maheshb; +Cc: j.vosburgh, andy, vfalico, nikolay, netdev, edumazet, maze
In-Reply-To: <1412469901-27451-1-git-send-email-maheshb@google.com>
From: Mahesh Bandewar <maheshb@google.com>
Date: Sat, 4 Oct 2014 17:45:01 -0700
> Earlier change to use usable slave array for TLB mode had an additional
> performance advantage. So extending the same logic to all other modes
> that use xmit-hash for slave selection (viz 802.3AD, and XOR modes).
> Also consolidating this with the earlier TLB change.
>
> The main idea is to build the usable slaves array in the control path
> and use that array for slave selection during xmit operation.
>
> Measured performance in a setup with a bond of 4x1G NICs with 200
> instances of netperf for the modes involved (3ad, xor, tlb)
> cmd: netperf -t TCP_RR -H <TargetHost> -l 60 -s 5
>
> Mode TPS-Before TPS-After
>
> 802.3ad : 468,694 493,101
> TLB (lb=0): 392,583 392,965
> XOR : 475,696 484,517
>
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] ipv4: igmp: fix v3 general query drop monitor false positive
From: David Miller @ 2014-10-06 21:15 UTC (permalink / raw)
To: dborkman; +Cc: edumazet, netdev
In-Reply-To: <1412522870-26335-1-git-send-email-dborkman@redhat.com>
From: Daniel Borkmann <dborkman@redhat.com>
Date: Sun, 5 Oct 2014 17:27:50 +0200
> In case we find a general query with non-zero number of sources, we
> are dropping the skb as it's malformed.
>
> RFC3376, section 4.1.8. Number of Sources (N):
>
> This number is zero in a General Query or a Group-Specific Query,
> and non-zero in a Group-and-Source-Specific Query.
>
> Therefore, reflect that by using kfree_skb() instead of consume_skb().
>
> Fixes: d679c5324d9a ("igmp: avoid drop_monitor false positives")
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Applied, thanks Daniel.
^ permalink raw reply
* Re: [PATCH net-next] net: introduce netdevice gso_min_segs attribute
From: David Miller @ 2014-10-06 21:21 UTC (permalink / raw)
To: eric.dumazet; +Cc: amirv, edumazet, netdev, yevgenyp, ogerlitz, idos
In-Reply-To: <1412529087.11091.14.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sun, 05 Oct 2014 10:11:27 -0700
> From: Eric Dumazet <edumazet@google.com>
>
> Some TSO engines might have a too heavy setup cost, that impacts
> performance on hosts sending small bursts (2 MSS per packet).
>
> This patch adds a device gso_min_segs, allowing drivers to set
> a minimum segment size for TSO packets, according to the NIC
> performance.
>
> Tested on a mlx4 NIC, this allows to get a ~110% increase of
> throughput when sending 2 MSS per packet.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
So exactly what value are you using for mlx4?
Because I wonder if we should just generically forfeit TSO unless
we have > 2 segments, for example.
^ permalink raw reply
* Re: [PATCH] net: Add ndo_gso_check
From: Tom Herbert @ 2014-10-06 21:28 UTC (permalink / raw)
To: Or Gerlitz
Cc: Alexander Duyck, John Fastabend, Jeff Kirsher, David Miller,
Linux Netdev List, Thomas Graf, Pravin Shelar, Andy Zhou
In-Reply-To: <CAJ3xEMiLsQ=Wd=461L1Wx0jQ+zivY-Z573+90dD8d7pdUD4Skw@mail.gmail.com>
On Mon, Oct 6, 2014 at 1:22 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> On Mon, Oct 6, 2014 at 8:59 PM, Tom Herbert <therbert@google.com> wrote:
>> On Sun, Oct 5, 2014 at 12:13 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>>> RX wise, Linux tells the driver that UDP port X would be used for
>>> VXLAN, right? and indeed, it's possible for some HW implementations
>>> not to support RX offloading (checksum) for both VXLAN and NVGRE @ the
>>> same time over the same port. But TX/GRO wise, you're probably
>>> correct. The thing is that from the user POV they need solution that
>>> works for both RX and TX offloading.
>
>> I think from a user POV we want a solution that supports RX and TX
>> offloading across the widest range of protocols. This is accomplished
>> by implementing protocol agnostic mechanisms like CHECKSUM_COMPLETE
>> and protocol agnostic UDP tunnel TSO like we've described. IMO, the
>> fact that we have devices that implement protocol specific mechanisms
>> for NVGRE and VXLAN should be considered legacy support in the stack,
>> for new UDP encapsulation protocols we should not expose specifics in
>> the stack in either by adding a GSO type for each protocol, nor
>> ndo_add_foo_port for each protocol-- these things will not scale and
>> unnecessarily complicate the core stack.
>
> I tend to generally agree to the wind that blows from your writeup, namely:
>
> UDP encapsulation offloads wise, we should pose few general
> requirements to NICs to be implemented by vendors in their tomorrow's
> HW and treat the current generation (these 4-5 drivers with their
> limitations as legacy which should be supported but not state the
> stack overall design).
>
> Still we should seek more ways to reduce the pain/amount of
> not-well-defined-configurations when these drivers are there and the
> stack goes through this upside-down turnaround changes. OTOH you
> didn't accept my SKB coloring suggestion for GSO inspection, and OTOH
> I guess we can live with some sort of generic helper in the form of
> what you suggested, but like it or not, getting rid of
> ndo_add_vxlan_port will simply break things out.
>
> Are we going to have a session on the encapsulation/offloads design @ LPC?
>
yes, I will talk about FOU and GUE implementation. You should
abstracts in the schedule now.
> I think a replay of your LKS presentation along with open discussion
> on how to get there with the legacy requirements could be very
> helpful.
>
>
> Or.
^ permalink raw reply
* Re: [net-next PATCH v1 1/3] net: sched: af_packet support for direct ring access
From: David Miller @ 2014-10-06 21:42 UTC (permalink / raw)
To: john.fastabend
Cc: dborkman, fw, gerlitz.or, hannes, netdev, john.ronciak, amirv,
eric.dumazet, danny.zhou
In-Reply-To: <20141006000629.32055.2295.stgit@nitbit.x32>
From: John Fastabend <john.fastabend@gmail.com>
Date: Sun, 05 Oct 2014 17:06:31 -0700
> This patch adds a net_device ops to split off a set of driver queues
> from the driver and map the queues into user space via mmap. This
> allows the queues to be directly manipulated from user space. For
> raw packet interface this removes any overhead from the kernel network
> stack.
About the facility in general, I am generally in favor, as I expressed
at the networking track in Chiacgo.
But you missed the mark wrt. describing the descriptors.
I do not want you to give device IDs.
I want the code to be %100 agnostic to device or vendor IDs.
Really "describe" the descriptor. Not just how large is it (32-bits,
64-bits, etc.), but also: 1) is it little or big endian 2) where is
the length field 3) where is control bit "foo" located, etc.
That's what I want to see in "struct tpacket_dev_info", rather than
device IDs and "versions".
^ permalink raw reply
* Re: [PATCH net-next] net: introduce netdevice gso_min_segs attribute
From: Eric Dumazet @ 2014-10-06 21:42 UTC (permalink / raw)
To: David Miller; +Cc: amirv, edumazet, netdev, yevgenyp, ogerlitz, idos
In-Reply-To: <20141006.172149.1596496098013953331.davem@davemloft.net>
On Mon, 2014-10-06 at 17:21 -0400, David Miller wrote:
> So exactly what value are you using for mlx4?
>
It seems that on ConnectX-3 family, TSO packets of 2 or 3 MSS are not
worth using TSO engine. The cutoff point seems to be 4 (same throughput)
So I was planning to use gso_min_segs = 4 only for them.
> Because I wonder if we should just generically forfeit TSO unless
> we have > 2 segments, for example.
When I tested on bnx2x, this was not a gain.
bnx2x is faster sending TSO packets, even if they have 2 MSS.
I'll try the experiment on I40E Intel cards.
^ permalink raw reply
* Re: [PATCH v2 net-next 15/15] tipc: remove old ASCII netlink API
From: Jon Paul Maloy @ 2014-10-06 21:47 UTC (permalink / raw)
To: jon.maloy@ericsson.com, richard.alpe@ericsson.com,
erik.hugne@ericsson.com, ying.xue@windriver.com
Cc: netdev@vger.kernel.org, tipc-discussion@lists.sourceforge.net
In-Reply-To: <20141006.152003.1261907320590776614.davem@davemloft.net>
I sort of expected that answer. Just resend the other ones so we get them in now (we are at rc7+). We can try to figure out if we can do a kernel-internal translation later.
///jon
Sent from Yahoo Mail on Android
------------------------------------------------------------------------------
Slashdot TV. Videos for Nerds. Stuff that Matters.
http://pubads.g.doubleclick.net/gampad/clk?id=160591471&iu=/4140/ostg.clktrk
^ permalink raw reply
* Re: [PATCH v2 net-next] net: better IFF_XMIT_DST_RELEASE support
From: David Miller @ 2014-10-06 21:50 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, ja
In-Reply-To: <1412559515.11091.46.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sun, 05 Oct 2014 18:38:35 -0700
> @@ -1001,7 +1001,8 @@ static netdev_features_t bond_fix_features(struct net_device *dev,
>
> static void bond_compute_features(struct bonding *bond)
> {
> - unsigned int flags, dst_release_flag = IFF_XMIT_DST_RELEASE;
> + unsigned int dst_release_flag = IFF_XMIT_DST_RELEASE |
> + IFF_XMIT_DST_RELEASE_PERM;
> netdev_features_t vlan_features = BOND_VLAN_FEATURES;
> netdev_features_t enc_features = BOND_ENC_FEATURES;
> struct net_device *bond_dev = bond->dev;
> @@ -1037,8 +1038,10 @@ done:
> bond_dev->gso_max_segs = gso_max_segs;
> netif_set_gso_max_size(bond_dev, gso_max_size);
>
> - flags = bond_dev->priv_flags & ~IFF_XMIT_DST_RELEASE;
> - bond_dev->priv_flags = flags | dst_release_flag;
> + bond_dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
> + if ((bond_dev->priv_flags & IFF_XMIT_DST_RELEASE_PERM) &&
> + dst_release_flag == (IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM))
> + bond_dev->priv_flags |= IFF_XMIT_DST_RELEASE;
I think I might be missing something, but in all of these places where
you add this logic, it looks to me like:
dst_release_flag = CONSTANT;
...
if (... &&
dst_release_flags == CONSTANT)
This 'dst_release_flag' variable never changes, so why bother with the
test at all?
Maybe I'm just being dense today...
^ permalink raw reply
* [PATCH net-next] net: core: Quiet W=1 warnings for unused vars and static functions
From: Joe Perches @ 2014-10-06 21:51 UTC (permalink / raw)
To: netdev; +Cc: LKML, John Fastabend
Reduce noise when compiling W=1.
All the variables are unused.
The functions are not called outside of the file so static
is preferred.
Signed-off-by: Joe Perches <joe@perches.com>
---
John, can you please verify that these gen_stats accesses
are unnecessary? I believe the compiler can elide them in
any case, but I'm not sure what you intended here.
net/core/dev.c | 4 ++--
net/core/gen_stats.c | 4 ----
net/core/rtnetlink.c | 3 +--
3 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 1a90530..2049a17 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5239,7 +5239,7 @@ void netdev_upper_dev_unlink(struct net_device *dev,
}
EXPORT_SYMBOL(netdev_upper_dev_unlink);
-void netdev_adjacent_add_links(struct net_device *dev)
+static void netdev_adjacent_add_links(struct net_device *dev)
{
struct netdev_adjacent *iter;
@@ -5264,7 +5264,7 @@ void netdev_adjacent_add_links(struct net_device *dev)
}
}
-void netdev_adjacent_del_links(struct net_device *dev)
+static void netdev_adjacent_del_links(struct net_device *dev)
{
struct netdev_adjacent *iter;
diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
index 14681b9..01be9cf 100644
--- a/net/core/gen_stats.c
+++ b/net/core/gen_stats.c
@@ -106,13 +106,9 @@ __gnet_stats_copy_basic_cpu(struct gnet_stats_basic_packed *bstats,
for_each_possible_cpu(i) {
struct gnet_stats_basic_cpu *bcpu = per_cpu_ptr(cpu, i);
unsigned int start;
- __u64 bytes;
- __u32 packets;
do {
start = u64_stats_fetch_begin_irq(&bcpu->syncp);
- bytes = bcpu->bstats.bytes;
- packets = bcpu->bstats.packets;
} while (u64_stats_fetch_retry_irq(&bcpu->syncp, start));
bstats->bytes += bcpu->bstats.bytes;
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a688268..c2fe350 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2917,7 +2917,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
{
struct net *net = sock_net(skb->sk);
rtnl_doit_func doit;
- int sz_idx, kind;
+ int kind;
int family;
int type;
int err;
@@ -2933,7 +2933,6 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
return 0;
family = ((struct rtgenmsg *)nlmsg_data(nlh))->rtgen_family;
- sz_idx = type>>2;
kind = type&3;
if (kind != 2 && !netlink_net_capable(skb, CAP_NET_ADMIN))
^ permalink raw reply related
* Re: [PATCH net-next v2] r8152: nway reset after setting eee
From: David Miller @ 2014-10-06 21:51 UTC (permalink / raw)
To: hayeswang; +Cc: netdev, nic_swsd, linux-kernel, linux-usb
In-Reply-To: <1394712342-15778-57-Taiwan-albertk@realtek.com>
From: Hayes Wang <hayeswang@realtek.com>
Date: Mon, 6 Oct 2014 10:36:04 +0800
> Restart autonegotiation is necessary after setting EEE.
>
> Signed-off-by: Hayes Wang <hayeswang@realtek.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH] net: Add ndo_gso_check
From: Jesse Gross @ 2014-10-06 21:51 UTC (permalink / raw)
To: Tom Herbert
Cc: Or Gerlitz, Alexander Duyck, John Fastabend, Jeff Kirsher,
David Miller, Linux Netdev List, Thomas Graf, Pravin Shelar,
Andy Zhou
In-Reply-To: <CA+mtBx8-w74f_f9JEfpwH9=Mjr_zaJJAYCMMdYAWdCy-51pzqg@mail.gmail.com>
On Sun, Oct 5, 2014 at 11:49 AM, Tom Herbert <therbert@google.com> wrote:
> On Sun, Oct 5, 2014 at 7:04 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>> On Thu, Oct 2, 2014 at 2:06 AM, Tom Herbert <therbert@google.com> wrote:
>>> On Wed, Oct 1, 2014 at 1:58 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>>>> On Tue, Sep 30, 2014 at 6:34 PM, Tom Herbert <therbert@google.com> wrote:
>>>>> On Tue, Sep 30, 2014 at 7:30 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>> [...]
>>> Solution #4: apply this patch and implement the check functions as
>>> needed in those 4 or 5 drivers. If a device can only do VXLAN/NVGRE
>>> then I believe the check function is something like:
>>>
>>> bool mydev_gso_check(struct sk_buff *skb, struct net_device *dev)
>>> {
>>> if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
>>> ((skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
>>> skb->protocol != htons(ETH_P_TEB) ||
>>> skb_inner_mac_header(skb) - skb_transport_header(skb) != 12)
>>> return false;
>>>
>>> return true;
>>> }
>>
>> Yep, such helper can can be basically made to work and let the 4-5
>> drivers that can
>> do GSO offloading for vxlan but not for any FOU/GUE packets signal
>> that to the stack.
>>
>> Re the 12 constant, you were referring to the udp+vxlan headers? it's 8+8
>>
>> Also, we need a way for drivers that can support VXLAN or NVGRE but
>> not concurrently
>> on the same port @ the same time to only let vxlan packet to pass
>> successfully through the helper.
>>
> Or, there should be no difference in GSO processing between VXLAN and
> NVGRE. Can you explain why you feel you need to differentiate them for
> GSO?
There is a difference in the processing that needs to happen for VXLAN
and GRE, even on transmit: at a minimum, the length field in the UDP
header needs to be updated for VXLAN. These are already broken out in
the stack between GRE and UDP tunnels anyways though.
^ permalink raw reply
* Re: [PATCH net-next] net: introduce netdevice gso_min_segs attribute
From: David Miller @ 2014-10-06 21:54 UTC (permalink / raw)
To: eric.dumazet; +Cc: amirv, edumazet, netdev, yevgenyp, ogerlitz, idos
In-Reply-To: <1412631778.11091.84.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 06 Oct 2014 14:42:58 -0700
> On Mon, 2014-10-06 at 17:21 -0400, David Miller wrote:
>
>> So exactly what value are you using for mlx4?
>>
>
> It seems that on ConnectX-3 family, TSO packets of 2 or 3 MSS are not
> worth using TSO engine. The cutoff point seems to be 4 (same throughput)
>
> So I was planning to use gso_min_segs = 4 only for them.
>
>> Because I wonder if we should just generically forfeit TSO unless
>> we have > 2 segments, for example.
>
> When I tested on bnx2x, this was not a gain.
>
> bnx2x is faster sending TSO packets, even if they have 2 MSS.
>
> I'll try the experiment on I40E Intel cards.
Ok I'm sold on your patch then if two major chipsets already benefit
from differing values.
I'll apply this, thanks Eric.
^ permalink raw reply
* Re: [PATCH net-next] net: core: Quiet W=1 warnings for unused vars and static functions
From: David Miller @ 2014-10-06 21:56 UTC (permalink / raw)
To: joe; +Cc: netdev, linux-kernel, john.fastabend
In-Reply-To: <1412632298.2916.42.camel@joe-AO725>
From: Joe Perches <joe@perches.com>
Date: Mon, 06 Oct 2014 14:51:38 -0700
> Reduce noise when compiling W=1.
>
> All the variables are unused.
> The functions are not called outside of the file so static
> is preferred.
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>
> John, can you please verify that these gen_stats accesses
> are unnecessary? I believe the compiler can elide them in
> any case, but I'm not sure what you intended here.
BTW, this patch reminds me that if people think there are
subdirectories where we can turn on things like -Werror in the
networking I would be very happy to apply such patches.
Things like arch/sparc has had this for years, I even forget when I
added it. :-)
Things like net/core/ for example should be doable for sure.
^ permalink raw reply
* Re: [PATCH net-next] net: core: Quiet W=1 warnings for unused vars and static functions
From: Eric Dumazet @ 2014-10-06 22:02 UTC (permalink / raw)
To: Joe Perches; +Cc: netdev, LKML, John Fastabend
In-Reply-To: <1412632298.2916.42.camel@joe-AO725>
On Mon, 2014-10-06 at 14:51 -0700, Joe Perches wrote:
> Reduce noise when compiling W=1.
>
> All the variables are unused.
> The functions are not called outside of the file so static
> is preferred.
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>
> John, can you please verify that these gen_stats accesses
> are unnecessary? I believe the compiler can elide them in
> any case, but I'm not sure what you intended here.
...
> diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
> index 14681b9..01be9cf 100644
> --- a/net/core/gen_stats.c
> +++ b/net/core/gen_stats.c
> @@ -106,13 +106,9 @@ __gnet_stats_copy_basic_cpu(struct gnet_stats_basic_packed *bstats,
> for_each_possible_cpu(i) {
> struct gnet_stats_basic_cpu *bcpu = per_cpu_ptr(cpu, i);
> unsigned int start;
> - __u64 bytes;
> - __u32 packets;
>
> do {
> start = u64_stats_fetch_begin_irq(&bcpu->syncp);
> - bytes = bcpu->bstats.bytes;
> - packets = bcpu->bstats.packets;
> } while (u64_stats_fetch_retry_irq(&bcpu->syncp, start));
>
Well... Please fix the bug for real.
> bstats->bytes += bcpu->bstats.bytes;
->
bstats->bytes += bytes;
bstats->packets += packets;
^ permalink raw reply
* Re: [net-next PATCH v1 0/3] net sched rcu updates
From: David Miller @ 2014-10-06 22:03 UTC (permalink / raw)
To: john.fastabend; +Cc: xiyou.wangcong, netdev, jhs, eric.dumazet
In-Reply-To: <20141006042335.6010.27000.stgit@nitbit.x32>
From: John Fastabend <john.fastabend@gmail.com>
Date: Sun, 05 Oct 2014 21:27:25 -0700
> This fixes the use of tcf_proto from RCU callbacks it requires
> moving the unbind calls out of the callbacks and removing the
> tcf_proto argument from the tcf_em_tree_destroy().
>
> This is a rework of two previous series and addresses comments
> from Cong. And should apply against latest net-next.
>
> The previous series links below for reference:
>
> (1/2) net: sched: do not use tcf_proto 'tp' argument from call_rcu
> http://patchwork.ozlabs.org/patch/396149/
>
> (2/2) net: sched: replace ematch calls to use struct net
> http://patchwork.ozlabs.org/patch/396150/
>
>
> net: sched: cls_cgroup tear down exts and ematch from rcu callback
> http://patchwork.ozlabs.org/patch/396307/
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next] net: core: Quiet W=1 warnings for unused vars and static functions
From: Joe Perches @ 2014-10-06 22:04 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-kernel, john.fastabend
In-Reply-To: <20141006.175612.1709169088283433326.davem@davemloft.net>
On Mon, 2014-10-06 at 17:56 -0400, David Miller wrote:
> From: Joe Perches <joe@perches.com>
> Date: Mon, 06 Oct 2014 14:51:38 -0700
>
> > Reduce noise when compiling W=1.
[]
> BTW, this patch reminds me that if people think there are
> subdirectories where we can turn on things like -Werror in the
> networking I would be very happy to apply such patches.
[]
> Things like net/core/ for example should be doable for sure.
I don't have any significant opposition to -Werror, but
I think there are real arguments _against_ using -Werror.
I think the primary one is new compiler versions have a
tendency to add new warnings for various things that can
unnecessarily and unpredictably break the build.
^ permalink raw reply
* Re: [PATCH v2 net-next] net: better IFF_XMIT_DST_RELEASE support
From: Eric Dumazet @ 2014-10-06 22:05 UTC (permalink / raw)
To: David Miller; +Cc: netdev, ja
In-Reply-To: <20141006.175015.2075737681876306575.davem@davemloft.net>
On Mon, 2014-10-06 at 17:50 -0400, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Sun, 05 Oct 2014 18:38:35 -0700
>
> > @@ -1001,7 +1001,8 @@ static netdev_features_t bond_fix_features(struct net_device *dev,
> >
> > static void bond_compute_features(struct bonding *bond)
> > {
> > - unsigned int flags, dst_release_flag = IFF_XMIT_DST_RELEASE;
> > + unsigned int dst_release_flag = IFF_XMIT_DST_RELEASE |
> > + IFF_XMIT_DST_RELEASE_PERM;
> > netdev_features_t vlan_features = BOND_VLAN_FEATURES;
> > netdev_features_t enc_features = BOND_ENC_FEATURES;
> > struct net_device *bond_dev = bond->dev;
> > @@ -1037,8 +1038,10 @@ done:
> > bond_dev->gso_max_segs = gso_max_segs;
> > netif_set_gso_max_size(bond_dev, gso_max_size);
> >
> > - flags = bond_dev->priv_flags & ~IFF_XMIT_DST_RELEASE;
> > - bond_dev->priv_flags = flags | dst_release_flag;
> > + bond_dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
> > + if ((bond_dev->priv_flags & IFF_XMIT_DST_RELEASE_PERM) &&
> > + dst_release_flag == (IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM))
> > + bond_dev->priv_flags |= IFF_XMIT_DST_RELEASE;
>
> I think I might be missing something, but in all of these places where
> you add this logic, it looks to me like:
>
> dst_release_flag = CONSTANT;
> ...
> if (... &&
> dst_release_flags == CONSTANT)
>
> This 'dst_release_flag' variable never changes, so why bother with the
> test at all?
We have a loop over team/bonding members, where we do :
dst_release_flag &= slave->dev->priv_flags;
So at the end of the loop, we check if any one of the member had one of
the bit cleared.
if dst_release_flags has both bits set, then we are set and we allow the
IFF_XMIT_DST_RELEASE being set on the master.
^ permalink raw reply
* Re: [PATCH net-next] net: core: Quiet W=1 warnings for unused vars and static functions
From: Cong Wang @ 2014-10-06 22:12 UTC (permalink / raw)
To: Joe Perches; +Cc: netdev, LKML, John Fastabend
In-Reply-To: <1412632298.2916.42.camel@joe-AO725>
On Mon, Oct 6, 2014 at 2:51 PM, Joe Perches <joe@perches.com> wrote:
> John, can you please verify that these gen_stats accesses
> are unnecessary? I believe the compiler can elide them in
> any case, but I'm not sure what you intended here.
>
> net/core/dev.c | 4 ++--
> net/core/gen_stats.c | 4 ----
> net/core/rtnetlink.c | 3 +--
> 3 files changed, 3 insertions(+), 8 deletions(-)
You need to split this patch.
> diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
> index 14681b9..01be9cf 100644
> --- a/net/core/gen_stats.c
> +++ b/net/core/gen_stats.c
> @@ -106,13 +106,9 @@ __gnet_stats_copy_basic_cpu(struct gnet_stats_basic_packed *bstats,
> for_each_possible_cpu(i) {
> struct gnet_stats_basic_cpu *bcpu = per_cpu_ptr(cpu, i);
> unsigned int start;
> - __u64 bytes;
> - __u32 packets;
>
> do {
> start = u64_stats_fetch_begin_irq(&bcpu->syncp);
> - bytes = bcpu->bstats.bytes;
> - packets = bcpu->bstats.packets;
> } while (u64_stats_fetch_retry_irq(&bcpu->syncp, start));
Looks like we really need them:
diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
index 14681b9..7948ecf 100644
--- a/net/core/gen_stats.c
+++ b/net/core/gen_stats.c
@@ -115,8 +115,8 @@ __gnet_stats_copy_basic_cpu(struct
gnet_stats_basic_packed *bstats,
packets = bcpu->bstats.packets;
} while (u64_stats_fetch_retry_irq(&bcpu->syncp, start));
- bstats->bytes += bcpu->bstats.bytes;
- bstats->packets += bcpu->bstats.packets;
+ bstats->bytes += bytes;
+ bstats->packets += packets;
}
}
^ permalink raw reply related
* Re: [PATCH v2 net-next] r8169:add support for RTL8168EP
From: Francois Romieu @ 2014-10-06 22:12 UTC (permalink / raw)
To: Hau; +Cc: netdev@vger.kernel.org, nic_swsd, linux-kernel@vger.kernel.org
In-Reply-To: <80377ECBC5453840BA8C7155328B537765786D@RTITMBSV03.realtek.com.tw>
Hau <hau@realtek.com> :
[...]
> Do you mean I should collect similar hardware parameters setting into one
> function ? or I should set hardware parameters according to hardware
> feature support version?
static void r8168dp_ocp_write(...)
[...]
static void r8168ep_ocp_write(...)
[...]
static void ocp_write(...)
{
switch (...
case ...
r8168dp_ocp_write
case ...
r8168ep_ocp_write
[...]
static void rtl8168dp_driver_start(...)
[...]
static void rtl8168ep_driver_start(...)
[...]
etc.
Nothing more. At some point the helpers themselves may turn into data
struct members. Things don't need to be immediately right - if ever.
However you really want to avoid unrelated changes in your patches:
shuffling code and changing features at the same time hurts reviews,
late regression hunts, backports, etc.
--
Ueimor
^ permalink raw reply
* Re: r8168 is needed to enter P-state: Package State 6 (pc6)onHaswell hardware
From: Francois Romieu @ 2014-10-06 22:13 UTC (permalink / raw)
To: Hayes Wang; +Cc: Ceriel Jacobs, nic_swsd, netdev@vger.kernel.org
In-Reply-To: <0835B3720019904CB8F7AA43166CEEB2526932@RTITMBSV03.realtek.com.tw>
Hayes Wang <hayeswang@realtek.com> :
> Francois Romieu [mailto:romieu@fr.zoreil.com]
[...]
> I don't sure if the following information is helpful. Besides, I remember
> the rtl_init_one() would disable it.
>
> http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=d64ec841517a25f6d468bde9f67e5b4cffdc67c7
>
> http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=4521e1a94279ce610d3f9b7945c17d581f804242
Yes, I did not expect this stuff to stay in geostationary orbit for long :o/
Realtek's r8168 driver defaults to CONFIG_ASPM=1 but I guess some users
need to disable it and there's no known pattern / blacklist, right ?
Ceriel, does the patch below against current kernel make a difference ?
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 0921302..b4a3881 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -468,6 +468,7 @@ enum rtl8168_registers {
#define PWM_EN (1 << 22)
#define RXDV_GATED_EN (1 << 19)
#define EARLY_TALLY_EN (1 << 16)
+#define FORCE_CLK (1 << 15) /* force clock request */
};
enum rtl_register_content {
@@ -5279,8 +5280,10 @@ static void rtl_hw_start_8168g_1(struct rtl8169_private *tp)
rtl_eri_write(tp, 0x2f8, ERIAR_MASK_0011, 0x1d8f, ERIAR_EXGMAC);
RTL_W8(ChipCmd, CmdTxEnb | CmdRxEnb);
- RTL_W32(MISC, RTL_R32(MISC) & ~RXDV_GATED_EN);
+ RTL_W32(MISC, (RTL_R32(MISC) | FORCE_CLK) & ~RXDV_GATED_EN);
RTL_W8(MaxTxPacketSize, EarlySize);
+ RTL_W8(Config5, RTL_R8(Config5) | ASPM_en);
+ RTL_W8(Config2, RTL_R8(Config2) | ClkReqEn);
rtl_eri_write(tp, 0xc0, ERIAR_MASK_0011, 0x0000, ERIAR_EXGMAC);
rtl_eri_write(tp, 0xb8, ERIAR_MASK_0011, 0x0000, ERIAR_EXGMAC);
^ permalink raw reply related
* Re: [PATCH net-next] icmp6: Add new icmpv6 type for RPL control message
From: David Miller @ 2014-10-06 22:13 UTC (permalink / raw)
To: simon.vincent; +Cc: netdev
In-Reply-To: <1412591826-32037-1-git-send-email-simon.vincent@xsilon.com>
From: Simon Vincent <simon.vincent@xsilon.com>
Date: Mon, 6 Oct 2014 11:37:06 +0100
> IANA has defined a type value of 155 for RPL control messages.
> We do nothing if we recieve one of these messages. This patch is to
> avoid getting lots of icmpv6 unknown type messages when using RPL.
>
> Signed-off-by: Simon Vincent <simon.vincent@xsilon.com>
If we agree that pretty much our policy is that we treat as "known"
any ICMPv6 type assigned officially by IANA, then we should simply
add everything missing from the table at:
http://www.iana.org/assignments/icmpv6-parameters/icmpv6-parameters.xhtml
Any objections?
^ 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