* Re: [PATCH 1/1] net: thunderx: avoid exposing kernel stack
From: Sunil Kovvuri @ 2016-05-09 18:57 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Sunil Goutham, Robert Richter, LAKML, Linux Netdev List, LKML
In-Reply-To: <1462747578-6147-1-git-send-email-xypron.glpk@gmx.de>
Thanks for fixing, changes look okay.
Sunil.
On Sun, May 8, 2016 at 3:46 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> Reserved fields should be set to zero to avoid exposing
> bits from the kernel stack.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> drivers/net/ethernet/cavium/thunder/nicvf_queues.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_queues.c b/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
> index 8acd7c0..0ff8e60 100644
> --- a/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
> @@ -533,6 +533,7 @@ static void nicvf_rcv_queue_config(struct nicvf *nic, struct queue_set *qs,
> nicvf_config_vlan_stripping(nic, nic->netdev->features);
>
> /* Enable Receive queue */
> + memset(&rq_cfg, 0, sizeof(struct rq_cfg));
> rq_cfg.ena = 1;
> rq_cfg.tcp_ena = 0;
> nicvf_queue_reg_write(nic, NIC_QSET_RQ_0_7_CFG, qidx, *(u64 *)&rq_cfg);
> @@ -565,6 +566,7 @@ void nicvf_cmp_queue_config(struct nicvf *nic, struct queue_set *qs,
> qidx, (u64)(cq->dmem.phys_base));
>
> /* Enable Completion queue */
> + memset(&cq_cfg, 0, sizeof(struct cq_cfg));
> cq_cfg.ena = 1;
> cq_cfg.reset = 0;
> cq_cfg.caching = 0;
> @@ -613,6 +615,7 @@ static void nicvf_snd_queue_config(struct nicvf *nic, struct queue_set *qs,
> qidx, (u64)(sq->dmem.phys_base));
>
> /* Enable send queue & set queue size */
> + memset(&sq_cfg, 0, sizeof(struct sq_cfg));
> sq_cfg.ena = 1;
> sq_cfg.reset = 0;
> sq_cfg.ldwb = 0;
> @@ -649,6 +652,7 @@ static void nicvf_rbdr_config(struct nicvf *nic, struct queue_set *qs,
>
> /* Enable RBDR & set queue size */
> /* Buffer size should be in multiples of 128 bytes */
> + memset(&rbdr_cfg, 0, sizeof(struct rbdr_cfg));
> rbdr_cfg.ena = 1;
> rbdr_cfg.reset = 0;
> rbdr_cfg.ldwb = 0;
> --
> 2.1.4
>
^ permalink raw reply
* Re: [PATCH net-next v2 00/21] net: dsa: mv88e6xxx: turn into monolithic driver
From: David Miller @ 2016-05-09 19:00 UTC (permalink / raw)
To: andrew; +Cc: vivien.didelot, netdev, linux-kernel, kernel, f.fainelli
In-Reply-To: <20160509175647.GE10508@lunn.ch>
From: Andrew Lunn <andrew@lunn.ch>
Date: Mon, 9 May 2016 19:56:47 +0200
> On Mon, May 09, 2016 at 01:22:37PM -0400, Vivien Didelot wrote:
>> This patchset merges all mv88e6* drivers supported by the shared
>> mv88e6xxx code into a single mv88e6xxx DSA switch driver.
>>
>> Some flags are added to describe the capabilities of a switch model,
>> such as the presence of a PPU, EEPROM, some old or new registers, etc.
>>
>> First these flags are used to conditionally support the same set of
>> functions in every driver, then specific driver files are removed in
>> favor of the common mv88e6xxx driver.
>>
>> Only the merge of driver specific setup code assumes a few differences.
>> If these differences such as frames priorities are really needed for
>> some models, they can easily be brought back in a future patch.
>>
>> Some inconsistencies might show up, such as the need for
>> MV88E6XXX_FLAG_PPU and MV88E6XXX_FLAG_PPU_ACTIVE flags. But this
>> patchset does not aim to fix them yet. A future patch can do that if
>> they end up being unwanted.
>>
>> The patchset has been tested on interconnected 88E6352 and 88E6185.
>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>
> I also tested v1, so
>
> Tested-by: Andrew Lunn <andrew@lunn.ch>
Series applied, thanks everyone.
^ permalink raw reply
* Re: [RESEND PATCH v6 0/6] Add Ethernet support on STM32F429
From: David Miller @ 2016-05-09 19:06 UTC (permalink / raw)
To: alexandre.torgue
Cc: mcoquelin.stm32, peppe.cavallaro, netdev, devicetree,
linux-arm-kernel, linux-kernel, robh, manabian, wens
In-Reply-To: <1462789899-13153-1-git-send-email-alexandre.torgue@gmail.com>
From: Alexandre TORGUE <alexandre.torgue@gmail.com>
Date: Mon, 9 May 2016 12:31:33 +0200
> STM32F429 Chip embeds a Synopsys 3.50a MAC IP.
> This series:
> -enhance current stmmac driver to control it (code already
> available) and adds basic glue for STM32F429 chip.
> -Enable basic Net config in kernel.
I assume this will go via the ARM tree, for the networking bits:
Acked-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* pull-request: wireless-drivers 2016-05-09
From: Kalle Valo @ 2016-05-09 19:06 UTC (permalink / raw)
To: David Miller; +Cc: linux-wireless, netdev, linux-kernel
Hi Dave,
one more iwlwifi fix we would like to get to 4.6 if possible. Please let
me know if there are any problems.
Kalle
The following changes since commit e2841ea91611622370f66b4dfaa62599e8850d48:
Merge tag 'iwlwifi-for-kalle-2016-04-12_2' of https://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-fixes (2016-04-21 15:40:13 +0300)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers.git tags/wireless-drivers-for-davem-2016-05-09
for you to fetch changes up to cbbba30f1ac95a04aae9c61a55aebe5e6fdf29f0:
Merge tag 'iwlwifi-for-kalle-2016-05-04' of https://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-fixes (2016-05-06 14:27:48 +0300)
----------------------------------------------------------------
wireless-drivers fixes for 4.6
iwlwifi
* fix P2P rates (and possibly other issues)
----------------------------------------------------------------
Emmanuel Grumbach (1):
iwlwifi: mvm: don't override the rate with the AMSDU len
Kalle Valo (1):
Merge tag 'iwlwifi-for-kalle-2016-05-04' of https://git.kernel.org/.../iwlwifi/iwlwifi-fixes
drivers/net/wireless/intel/iwlwifi/mvm/tx.c | 83 ++++++++++++++++-----------
1 file changed, 48 insertions(+), 35 deletions(-)
^ permalink raw reply
* Re: [PATCH net-next 01/14] qed: Add CONFIG_QED_SRIOV
From: David Miller @ 2016-05-09 19:14 UTC (permalink / raw)
To: Yuval.Mintz; +Cc: netdev, Ariel.Elior
In-Reply-To: <1462799963-23547-2-git-send-email-Yuval.Mintz@qlogic.com>
From: Yuval Mintz <Yuval.Mintz@qlogic.com>
Date: Mon, 9 May 2016 16:19:10 +0300
> + /* bitmap indicating which fields hold valid values */
> + aligned_u64 valid_bitmap;
There is absolutely no reason to use aligned_u64 here. That type is for
handling a specific issue in user facing APIs, which this is not.
^ permalink raw reply
* Re: [PATCH 00/36] Netfilter updates for net-next
From: David Miller @ 2016-05-09 19:15 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel, netdev
In-Reply-To: <1462819614-5402-1-git-send-email-pablo@netfilter.org>
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Mon, 9 May 2016 20:46:18 +0200
> The following large patchset contains Netfilter updates for your
> net-next tree. My initial intention was to send you this in two goes but
> when I looked back twice I already had this burden on top of me.
Pulled, thanks Pablo.
^ permalink raw reply
* [PATCH net-next 1/2] net: sched: Move TCA_CLS_FLAGS_SKIP_HW to uapi header file.
From: Sridhar Samudrala @ 2016-05-09 19:18 UTC (permalink / raw)
To: john.r.fastabend, netdev
This enables the flag to be shared with userspace(iproute2 etc).
Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
---
include/net/pkt_cls.h | 3 ---
include/uapi/linux/pkt_cls.h | 3 +++
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index caa5e18..339ef08 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -392,9 +392,6 @@ struct tc_cls_u32_offload {
};
};
-/* tca flags definitions */
-#define TCA_CLS_FLAGS_SKIP_HW 1
-
static inline bool tc_should_offload(struct net_device *dev, u32 flags)
{
if (!(dev->features & NETIF_F_HW_TC))
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 8466090..3e8b65f 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -151,6 +151,9 @@ enum {
#define TCA_POLICE_MAX (__TCA_POLICE_MAX - 1)
+/* tca flags definitions */
+#define TCA_CLS_FLAGS_SKIP_HW (1 << 0)
+
/* U32 filters */
#define TC_U32_HTID(h) ((h)&0xFFF00000)
--
2.1.0
^ permalink raw reply related
* [PATCH net-next 2/2] net: cls_u32: Add support for skip-sw flag to tc u32 classifier.
From: Sridhar Samudrala @ 2016-05-09 19:18 UTC (permalink / raw)
To: john.r.fastabend, netdev
In-Reply-To: <1462821524-2534-1-git-send-email-sridhar.samudrala@intel.com>
On devices that support TC U32 offloads, this flag enables a filter to be
added only to HW. skip-sw and skip-hw are mutually exclusive flags. By
default without any flags, the filter is added to both HW and SW, but no
error checks are done in case of failure to add to HW. With skip-sw,
failure to add to HW is treated as an error.
Here is a sample script that adds 2 filters, one with skip-sw and the other
with skip-hw flag.
# add ingress qdisc
tc qdisc add dev p4p1 ingress
# enable hw tc offload.
ethtool -K p4p1 hw-tc-offload on
# add u32 filter with skip-sw flag.
tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
handle 800:0:1 u32 ht 800: flowid 800:1 \
skip-sw \
match ip src 192.168.1.0/24 \
action drop
# add u32 filter with skip-hw flag.
tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
handle 800:0:2 u32 ht 800: flowid 800:2 \
skip-hw \
match ip src 192.168.2.0/24 \
action drop
Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
---
include/net/pkt_cls.h | 17 +++++++++++++++++
include/uapi/linux/pkt_cls.h | 1 +
net/sched/cls_u32.c | 45 +++++++++++++++++++++++++++++++++++---------
3 files changed, 54 insertions(+), 9 deletions(-)
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 339ef08..8b48938 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -406,6 +406,23 @@ static inline bool tc_should_offload(struct net_device *dev, u32 flags)
return true;
}
+static inline bool tc_skip_sw(u32 flags)
+{
+ return (flags & TCA_CLS_FLAGS_SKIP_SW) ? true : false;
+}
+
+/* SKIP_HW and SKIP_SW are mutually exclusive flags. */
+static inline bool tc_flags_valid(u32 flags)
+{
+ if (flags & ~(TCA_CLS_FLAGS_SKIP_HW | TCA_CLS_FLAGS_SKIP_SW))
+ return false;
+
+ if (!(flags ^ (TCA_CLS_FLAGS_SKIP_HW | TCA_CLS_FLAGS_SKIP_SW)))
+ return false;
+
+ return true;
+}
+
enum tc_fl_command {
TC_CLSFLOWER_REPLACE,
TC_CLSFLOWER_DESTROY,
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 3e8b65f..eba5914 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -153,6 +153,7 @@ enum {
/* tca flags definitions */
#define TCA_CLS_FLAGS_SKIP_HW (1 << 0)
+#define TCA_CLS_FLAGS_SKIP_SW (1 << 1)
/* U32 filters */
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index e64877a..079b43b 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -134,6 +134,11 @@ next_knode:
j = 0;
#endif
+ if (tc_skip_sw(n->flags)) {
+ n = rcu_dereference_bh(n->next);
+ goto next_knode;
+ }
+
#ifdef CONFIG_CLS_U32_MARK
if ((skb->mark & n->mask) != n->val) {
n = rcu_dereference_bh(n->next);
@@ -443,13 +448,14 @@ static void u32_remove_hw_knode(struct tcf_proto *tp, u32 handle)
}
}
-static void u32_replace_hw_hnode(struct tcf_proto *tp,
+static int u32_replace_hw_hnode(struct tcf_proto *tp,
struct tc_u_hnode *h,
u32 flags)
{
struct net_device *dev = tp->q->dev_queue->dev;
struct tc_cls_u32_offload u32_offload = {0};
struct tc_to_netdev offload;
+ int err;
offload.type = TC_SETUP_CLSU32;
offload.cls_u32 = &u32_offload;
@@ -460,9 +466,13 @@ static void u32_replace_hw_hnode(struct tcf_proto *tp,
offload.cls_u32->hnode.handle = h->handle;
offload.cls_u32->hnode.prio = h->prio;
- dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle,
- tp->protocol, &offload);
+ err = dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle,
+ tp->protocol, &offload);
+ if (tc_skip_sw(flags))
+ return err;
}
+
+ return 0;
}
static void u32_clear_hw_hnode(struct tcf_proto *tp, struct tc_u_hnode *h)
@@ -485,13 +495,14 @@ static void u32_clear_hw_hnode(struct tcf_proto *tp, struct tc_u_hnode *h)
}
}
-static void u32_replace_hw_knode(struct tcf_proto *tp,
+static int u32_replace_hw_knode(struct tcf_proto *tp,
struct tc_u_knode *n,
u32 flags)
{
struct net_device *dev = tp->q->dev_queue->dev;
struct tc_cls_u32_offload u32_offload = {0};
struct tc_to_netdev offload;
+ int err;
offload.type = TC_SETUP_CLSU32;
offload.cls_u32 = &u32_offload;
@@ -512,9 +523,13 @@ static void u32_replace_hw_knode(struct tcf_proto *tp,
if (n->ht_down)
offload.cls_u32->knode.link_handle = n->ht_down->handle;
- dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle,
- tp->protocol, &offload);
+ err = dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle,
+ tp->protocol, &offload);
+ if (tc_skip_sw(flags))
+ return err;
}
+
+ return 0;
}
static void u32_clear_hnode(struct tcf_proto *tp, struct tc_u_hnode *ht)
@@ -845,8 +860,11 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
if (err < 0)
return err;
- if (tb[TCA_U32_FLAGS])
+ if (tb[TCA_U32_FLAGS]) {
flags = nla_get_u32(tb[TCA_U32_FLAGS]);
+ if (!tc_flags_valid(flags))
+ return err;
+ }
n = (struct tc_u_knode *)*arg;
if (n) {
@@ -871,10 +889,15 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
return err;
}
+ err = u32_replace_hw_knode(tp, new, flags);
+ if (err) {
+ u32_destroy_key(tp, new, false);
+ return err;
+ }
+
u32_replace_knode(tp, tp_c, new);
tcf_unbind_filter(tp, &n->res);
call_rcu(&n->rcu, u32_delete_key_rcu);
- u32_replace_hw_knode(tp, new, flags);
return 0;
}
@@ -978,6 +1001,10 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
struct tc_u_knode __rcu **ins;
struct tc_u_knode *pins;
+ err = u32_replace_hw_knode(tp, n, flags);
+ if (err)
+ goto errhw;
+
ins = &ht->ht[TC_U32_HASH(handle)];
for (pins = rtnl_dereference(*ins); pins;
ins = &pins->next, pins = rtnl_dereference(*ins))
@@ -986,11 +1013,11 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
RCU_INIT_POINTER(n->next, pins);
rcu_assign_pointer(*ins, n);
- u32_replace_hw_knode(tp, n, flags);
*arg = (unsigned long)n;
return 0;
}
+errhw:
#ifdef CONFIG_CLS_U32_MARK
free_percpu(n->pcpu_success);
errout:
--
2.1.0
^ permalink raw reply related
* Re: NFSroot hangs with bad unlock balance in Linux next
From: Tony Lindgren @ 2016-05-09 19:40 UTC (permalink / raw)
To: Al Viro
Cc: Eric Dumazet, linux-nfs, Jiri Pirko, netdev, Trond Myklebust,
Christoph Hellwig, linux-omap, Anna Schumaker, linux-arm-kernel
In-Reply-To: <20160509153932.GJ2694@ZenIV.linux.org.uk>
* Al Viro <viro@ZenIV.linux.org.uk> [160509 08:41]:
> On Mon, May 09, 2016 at 08:21:38AM -0700, Tony Lindgren wrote:
>
> > Looks like with both patches applied I still also get this eventually:
> >
> > =====================================
> > [ BUG: bad unlock balance detected! ]
> > 4.6.0-rc7-next-20160509+ #1264 Not tainted
> > -------------------------------------
>
> Lockdep warnings are noise. To make them STFU try the following incremental;
> I'll fold it into #work.lookups and #for-next. Note that it will do nothing
> to hangs - those are completely unrelated and you need Eric's patch to deal
> with them.
OK yeah that helps thanks:
Tested-by: Tony Lindgren <tony@atomide.com>
> diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c
> index d367b06..1868246 100644
> --- a/fs/nfs/unlink.c
> +++ b/fs/nfs/unlink.c
> @@ -64,7 +64,7 @@ static void nfs_async_unlink_release(void *calldata)
> struct dentry *dentry = data->dentry;
> struct super_block *sb = dentry->d_sb;
>
> - up_read(&NFS_I(d_inode(dentry->d_parent))->rmdir_sem);
> + up_read_non_owner(&NFS_I(d_inode(dentry->d_parent))->rmdir_sem);
> d_lookup_done(dentry);
> nfs_free_unlinkdata(data);
> dput(dentry);
> @@ -117,10 +117,10 @@ static int nfs_call_unlink(struct dentry *dentry, struct nfs_unlinkdata *data)
> struct inode *dir = d_inode(dentry->d_parent);
> struct dentry *alias;
>
> - down_read(&NFS_I(dir)->rmdir_sem);
> + down_read_non_owner(&NFS_I(dir)->rmdir_sem);
> alias = d_alloc_parallel(dentry->d_parent, &data->args.name, &data->wq);
> if (IS_ERR(alias)) {
> - up_read(&NFS_I(dir)->rmdir_sem);
> + up_read_non_owner(&NFS_I(dir)->rmdir_sem);
> return 0;
> }
> if (!d_in_lookup(alias)) {
> @@ -142,7 +142,7 @@ static int nfs_call_unlink(struct dentry *dentry, struct nfs_unlinkdata *data)
> ret = 0;
> spin_unlock(&alias->d_lock);
> dput(alias);
> - up_read(&NFS_I(dir)->rmdir_sem);
> + up_read_non_owner(&NFS_I(dir)->rmdir_sem);
> /*
> * If we'd displaced old cached devname, free it. At that
> * point dentry is definitely not a root, so we won't need
^ permalink raw reply
* [PATCH] netfilter: conntrack: remove uninitialized shadow variable
From: Arnd Bergmann @ 2016-05-09 19:47 UTC (permalink / raw)
To: Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
David S. Miller
Cc: Arnd Bergmann, Florian Westphal, Daniel Borkmann, Sasha Levin,
netfilter-devel, coreteam, netdev, linux-kernel
A recent commit introduced an unconditional use of an uninitialized
variable, as reported in this gcc warning:
net/netfilter/nf_conntrack_core.c: In function '__nf_conntrack_confirm':
net/netfilter/nf_conntrack_core.c:632:33: error: 'ctinfo' may be used uninitialized in this function [-Werror=maybe-uninitialized]
bytes = atomic64_read(&counter[CTINFO2DIR(ctinfo)].bytes);
^
net/netfilter/nf_conntrack_core.c:628:26: note: 'ctinfo' was declared here
enum ip_conntrack_info ctinfo;
The problem is that a local variable shadows the function parameter.
This removes the local variable, which looks like what Pablo originally
intended.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 71d8c47fc653 ("netfilter: conntrack: introduce clash resolution on insertion race")
---
net/netfilter/nf_conntrack_core.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index f58a70410c69..157ffa667395 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -625,7 +625,6 @@ static void nf_ct_acct_merge(struct nf_conn *ct, enum ip_conntrack_info ctinfo,
acct = nf_conn_acct_find(loser_ct);
if (acct) {
struct nf_conn_counter *counter = acct->counter;
- enum ip_conntrack_info ctinfo;
unsigned int bytes;
/* u32 should be fine since we must have seen one packet. */
--
2.7.0
^ permalink raw reply related
* [PATCH 2/3] netfilter: IDLETIMER: fix race condition when destroy the target
From: Pablo Neira Ayuso @ 2016-05-09 19:52 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1462823523-6682-1-git-send-email-pablo@netfilter.org>
From: Liping Zhang <liping.zhang@spreadtrum.com>
Workqueue maybe still in running while we destroy the IDLETIMER target,
thus cause a use after free error, add cancel_work_sync() to avoid such
situation.
Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/xt_IDLETIMER.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/netfilter/xt_IDLETIMER.c b/net/netfilter/xt_IDLETIMER.c
index 29d2c31..daf45da 100644
--- a/net/netfilter/xt_IDLETIMER.c
+++ b/net/netfilter/xt_IDLETIMER.c
@@ -236,6 +236,7 @@ static void idletimer_tg_destroy(const struct xt_tgdtor_param *par)
list_del(&info->timer->entry);
del_timer_sync(&info->timer->timer);
+ cancel_work_sync(&info->timer->work);
sysfs_remove_file(idletimer_tg_kobj, &info->timer->attr.attr);
kfree(info->timer->attr.attr.name);
kfree(info->timer);
--
2.1.4
^ permalink raw reply related
* [PATCH 3/3] netfilter: nfnetlink_acct: validate NFACCT_QUOTA parameter
From: Pablo Neira Ayuso @ 2016-05-09 19:52 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1462823523-6682-1-git-send-email-pablo@netfilter.org>
From: Phil Turnbull <phil.turnbull@oracle.com>
If a quota bit is set in NFACCT_FLAGS but the NFACCT_QUOTA parameter is
missing then a NULL pointer dereference is triggered. CAP_NET_ADMIN is
required to trigger the bug.
Signed-off-by: Phil Turnbull <phil.turnbull@oracle.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/nfnetlink_acct.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
index 4c2b4c0..dbd0803 100644
--- a/net/netfilter/nfnetlink_acct.c
+++ b/net/netfilter/nfnetlink_acct.c
@@ -96,6 +96,8 @@ static int nfnl_acct_new(struct net *net, struct sock *nfnl,
return -EINVAL;
if (flags & NFACCT_F_OVERQUOTA)
return -EINVAL;
+ if ((flags & NFACCT_F_QUOTA) && !tb[NFACCT_QUOTA])
+ return -EINVAL;
size += sizeof(u64);
}
--
2.1.4
^ permalink raw reply related
* [PATCH 1/3] netfilter: conntrack: init all_locks to avoid debug warning
From: Pablo Neira Ayuso @ 2016-05-09 19:52 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1462823523-6682-1-git-send-email-pablo@netfilter.org>
From: Florian Westphal <fw@strlen.de>
Else we get 'BUG: spinlock bad magic on CPU#' on resize when
spin lock debugging is enabled.
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/nf_conntrack_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index afde5f5..895d11d 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -66,7 +66,7 @@ EXPORT_SYMBOL_GPL(nf_conntrack_locks);
__cacheline_aligned_in_smp DEFINE_SPINLOCK(nf_conntrack_expect_lock);
EXPORT_SYMBOL_GPL(nf_conntrack_expect_lock);
-static __read_mostly spinlock_t nf_conntrack_locks_all_lock;
+static __read_mostly DEFINE_SPINLOCK(nf_conntrack_locks_all_lock);
static __read_mostly bool nf_conntrack_locks_all;
void nf_conntrack_lock(spinlock_t *lock) __acquires(lock)
--
2.1.4
^ permalink raw reply related
* [PATCH 0/3] Netfilter fixes for net
From: Pablo Neira Ayuso @ 2016-05-09 19:52 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
Hi David,
The following patchset contain Netfilter simple fixes for your net tree,
two one-liner and one two-liner:
1) Oneliner to fix missing spinlock definition that triggers
'BUG: spinlock bad magic on CPU#' when spinlock debugging is enabled,
from Florian Westphal.
2) Fix missing workqueue cancelation on IDLETIMER removal,
from Liping Zhang.
3) Fix insufficient validation of netlink of NFACCT_QUOTA in
nfnetlink_acct, from Phil Turnbull.
You can pull these changes from:
git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git
Thanks!
----------------------------------------------------------------
The following changes since commit f28f20da704d399fb1e4d8838ffd697a357d9cc8:
Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net (2016-04-26 16:25:51 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git HEAD
for you to fetch changes up to eda3fc50daa93b08774a18d51883c5a5d8d85e15:
netfilter: nfnetlink_acct: validate NFACCT_QUOTA parameter (2016-05-05 16:47:08 +0200)
----------------------------------------------------------------
Florian Westphal (1):
netfilter: conntrack: init all_locks to avoid debug warning
Liping Zhang (1):
netfilter: IDLETIMER: fix race condition when destroy the target
Phil Turnbull (1):
netfilter: nfnetlink_acct: validate NFACCT_QUOTA parameter
net/netfilter/nf_conntrack_core.c | 2 +-
net/netfilter/nfnetlink_acct.c | 2 ++
net/netfilter/xt_IDLETIMER.c | 1 +
3 files changed, 4 insertions(+), 1 deletion(-)
^ permalink raw reply
* Re: [net-next PATCH V1 1/3] net: bulk alloc and reuse of SKBs in NAPI context
From: Jesper Dangaard Brouer @ 2016-05-09 19:59 UTC (permalink / raw)
To: Alexander Duyck
Cc: Netdev, David S. Miller, Saeed Mahameed, Or Gerlitz,
Eugenia Emantayev, brouer
In-Reply-To: <CAKgT0Uftr643AR9n2=_aQmaGJO3eEyKTuaCfXwEKbYj1rVruRw@mail.gmail.com>
On Mon, 9 May 2016 09:20:41 -0700
Alexander Duyck <alexander.duyck@gmail.com> wrote:
> On Mon, May 9, 2016 at 6:44 AM, Jesper Dangaard Brouer
> <brouer@redhat.com> wrote:
> > This patch introduce bulk alloc of SKBs and allow reuse of SKBs
> > free'ed in same softirq cycle. SKBs are normally free'ed during TX
> > completion, but most high speed drivers also cleanup TX ring during
> > NAPI RX poll cycle. Thus, if using napi_consume_skb/__kfree_skb_defer,
> > SKBs will be avail in the napi_alloc_cache->skb_cache.
> >
> > If no SKBs are avail for reuse, then only bulk alloc 8 SKBs, to limit
> > the potential overshooting unused SKBs needed to free'ed when NAPI
> > cycle ends (flushed in net_rx_action via __kfree_skb_flush()).
> >
> > Generalize ___build_skb() to allow passing it a preallocated SKB.
> >
> > I've previously demonstrated a 1% speedup for IPv4 forwarding, when
> > used on the ixgbe driver. If SKB alloc and free happens on different
> > CPUs (like in RPS use-case) the performance benefit is expected to
> > increase.
>
> Really I am not sure this patch series is worth the effort. For
> freeing buffers in Tx it was an obvious win. But adding all this
> complexity for a 1% gain just doesn't seen worth the effort.
I still think it is worth it, because: 1) it enables recycling, which
is more likely for real-life traffic (e.g. some TCP ACKs gets TX DMA
completion cleanup, and RX can re-use these), and 2) because bulk alloc
and bulk free gets "coupled" (mostly relevant when doing cross CPU).
> > All drivers using the napi_alloc_skb() call will benefit from
> > this change automatically.
>
> Yes, but a 1% improvement is essentially just noise. I'd say we need
> to show a better gain or be able to more precisely show that this is a
> definite gain and not just a test fluctuation resulting in the
> improvement. For all I know all of the gain could be due to a
> function shifting around so that some loop is now 16 byte aligned.
>
> > Joint work with Alexander Duyck.
> >
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
>
> The fact that this is still using my redhat.com address says volumes.
> If I recall I think we were working on this code around 9 months ago
> and had similar issues with it showing either negative performance or
> no gain. My advice then was to push the bulk free code and try to
> find a way to fix the bulk allocation. If we are still at this state
> for bulk allocation then maybe we need to drop the bulk allocation and
> start looking at other avenues to pursue.
I'm sure there is a gain here. Sure I can spend 2-3 weeks coming up
with a benchmark that show a bigger gain, but is it time well spend?
My quick benchmarking with mlx4 actually showed 2.7% improvement, for
local UDP socket delivery. And only 0.6% on IP-forwarding. If I find
some bidirectional traffic then the benefit should be higher due to
recycling kicking in.
We did do as you recommended, and the bulk free code first. I'm
just getting the last pieces pushed. I didn't see any negative
performance in my testing.
As you also know, tuning the SLUB system will give higher performance,
easily. In the future, I'm planning to get some auto-tuning into the
SLUB allocator. I've already discussed this with Christiph Lameter, at
MM-summit, see presentation[1] slides 4 and 5.
[1] http://people.netfilter.org/hawk/presentations/MM-summit2016/slab_mm_summit2016.odp
> > ---
> > net/core/skbuff.c | 71
> > ++++++++++++++++++++++++++++++++++------------------- 1 file
> > changed, 45 insertions(+), 26 deletions(-)
> >
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 5586be93632f..e85f1065b263 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -281,32 +281,14 @@ nodata:
> > }
> > EXPORT_SYMBOL(__alloc_skb);
> >
> > -/**
> > - * __build_skb - build a network buffer
> > - * @data: data buffer provided by caller
> > - * @frag_size: size of data, or 0 if head was kmalloced
> > - *
> > - * Allocate a new &sk_buff. Caller provides space holding head and
> > - * skb_shared_info. @data must have been allocated by kmalloc()
> > only if
> > - * @frag_size is 0, otherwise data should come from the page
> > allocator
> > - * or vmalloc()
> > - * The return is the new skb buffer.
> > - * On a failure the return is %NULL, and @data is not freed.
> > - * Notes :
> > - * Before IO, driver allocates only data buffer where NIC put
> > incoming frame
> > - * Driver should add room at head (NET_SKB_PAD) and
> > - * MUST add room at tail (SKB_DATA_ALIGN(skb_shared_info))
> > - * After IO, driver calls build_skb(), to allocate sk_buff and
> > populate it
> > - * before giving packet to stack.
> > - * RX rings only contains data buffers, not full skbs.
> > - */
> > -struct sk_buff *__build_skb(void *data, unsigned int frag_size)
> > +/* Allows skb being (pre)allocated by caller */
> > +static inline
> > +struct sk_buff *___build_skb(void *data, unsigned int frag_size,
> > + struct sk_buff *skb)
> > {
> > struct skb_shared_info *shinfo;
> > - struct sk_buff *skb;
> > unsigned int size = frag_size ? : ksize(data);
> >
> > - skb = kmem_cache_alloc(skbuff_head_cache, GFP_ATOMIC);
> > if (!skb)
> > return NULL;
> >
> > @@ -331,6 +313,34 @@ struct sk_buff *__build_skb(void *data,
> > unsigned int frag_size) return skb;
> > }
> >
> > +/**
> > + * __build_skb - build a network buffer
> > + * @data: data buffer provided by caller
> > + * @frag_size: size of data, or 0 if head was kmalloced
> > + *
> > + * Allocate a new &sk_buff. Caller provides space holding head and
> > + * skb_shared_info. @data must have been allocated by kmalloc()
> > only if
> > + * @frag_size is 0, otherwise data should come from the page
> > allocator
> > + * or vmalloc()
> > + * The return is the new skb buffer.
> > + * On a failure the return is %NULL, and @data is not freed.
> > + * Notes :
> > + * Before IO, driver allocates only data buffer where NIC put
> > incoming frame
> > + * Driver should add room at head (NET_SKB_PAD) and
> > + * MUST add room at tail (SKB_DATA_ALIGN(skb_shared_info))
> > + * After IO, driver calls build_skb(), to allocate sk_buff and
> > populate it
> > + * before giving packet to stack.
> > + * RX rings only contains data buffers, not full skbs.
> > + */
> > +struct sk_buff *__build_skb(void *data, unsigned int frag_size)
> > +{
> > + struct sk_buff *skb;
> > + unsigned int size = frag_size ? : ksize(data);
> > +
> > + skb = kmem_cache_alloc(skbuff_head_cache, GFP_ATOMIC);
> > + return ___build_skb(data, size, skb);
> > +}
> > +
> > /* build_skb() is wrapper over __build_skb(), that specifically
> > * takes care of skb->head and skb->pfmemalloc
> > * This means that if @frag_size is not zero, then @data must be
> > backed
>
> If we can avoid having to break up build_skb into more functions that
> would be preferred. I realize I probably wrote this code in order to
> enable the bulk allocation approach, but really I don't want to add
> more complexity unless we can show a strong gain which we haven't been
> able to demonstrate.
You do notice that ___build_skb gets inlined, thus there is not
performance loss. And this change was explicitly requested last time
this patch was reviewed. And I think this variant is much less
intrusive.
> > @@ -490,8 +500,8 @@ struct sk_buff *__napi_alloc_skb(struct
> > napi_struct *napi, unsigned int len,
> >
> > len += NET_SKB_PAD + NET_IP_ALIGN;
> >
> > - if ((len > SKB_WITH_OVERHEAD(PAGE_SIZE)) ||
> > - (gfp_mask & (__GFP_DIRECT_RECLAIM | GFP_DMA))) {
> > + if (unlikely((len > SKB_WITH_OVERHEAD(PAGE_SIZE)) ||
> > + (gfp_mask & (__GFP_DIRECT_RECLAIM |
> > GFP_DMA)))) { skb = __alloc_skb(len, gfp_mask, SKB_ALLOC_RX,
> > NUMA_NO_NODE); if (!skb)
> > goto skb_fail;
>
> Does this unlikely really make a difference? If so you might want to
> move it into a patch all on its own.
I can place it in a patch of it's own. I just noticed the compiler
layed out the code wrongly compared to the normal use-case.
> > @@ -508,11 +518,20 @@ struct sk_buff *__napi_alloc_skb(struct
> > napi_struct *napi, unsigned int len, if (unlikely(!data))
> > return NULL;
> >
> > - skb = __build_skb(data, len);
> > - if (unlikely(!skb)) {
> > +#define BULK_ALLOC_SIZE 8
> > + if (!nc->skb_count) {
> > + nc->skb_count =
> > kmem_cache_alloc_bulk(skbuff_head_cache,
> > + gfp_mask,
> > BULK_ALLOC_SIZE,
> > +
> > nc->skb_cache);
> > + }
> > + if (likely(nc->skb_count)) {
> > + skb = (struct sk_buff
> > *)nc->skb_cache[--nc->skb_count];
> > + } else {
> > + /* alloc bulk failed */
>
> So did you try doing a low latency socket test with this patch? I'm
> guessing not as I am certain there is a negative impact from having to
> allocate 8 buffers, and then free back 7 every time you call the NAPI
> polling routine with just one buffer in the ring.
There is a very high probability that pulling out 8 objects, and
returning 7 object, will have the same cost of alloc and free of a
single object. This is due to how the SLUB allocator's per CPU
allocator works.
Notice I said "high probability". I have adjusted the slab bulk APIs,
such that we can extend them to bulk return "upto" a given number of
objects. Then the SLUB allocator can remove the "high probability"
part, and make sure only to return object on the per CPU slab-page,
thus guaranteeing no cmpxchg_double calls, and only
local_irq_disable/enable cycles, which is actually faster than the
normal fastpath local cmpxchg_double (non-atomic variant).
> > skb_free_frag(data);
> > return NULL;
> > }
> > + skb = ___build_skb(data, len, skb);
> >
> > /* use OR instead of assignment to avoid clearing of bits
> > in mask */ if (nc->page.pfmemalloc)
> >
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH] netfilter: conntrack: remove uninitialized shadow variable
From: Pablo Neira Ayuso @ 2016-05-09 20:01 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Patrick McHardy, Jozsef Kadlecsik, David S. Miller,
Florian Westphal, Daniel Borkmann, Sasha Levin, netfilter-devel,
coreteam, netdev, linux-kernel
In-Reply-To: <1462823254-3137116-1-git-send-email-arnd@arndb.de>
On Mon, May 09, 2016 at 09:47:23PM +0200, Arnd Bergmann wrote:
> A recent commit introduced an unconditional use of an uninitialized
> variable, as reported in this gcc warning:
>
> net/netfilter/nf_conntrack_core.c: In function '__nf_conntrack_confirm':
> net/netfilter/nf_conntrack_core.c:632:33: error: 'ctinfo' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> bytes = atomic64_read(&counter[CTINFO2DIR(ctinfo)].bytes);
> ^
> net/netfilter/nf_conntrack_core.c:628:26: note: 'ctinfo' was declared here
> enum ip_conntrack_info ctinfo;
>
> The problem is that a local variable shadows the function parameter.
> This removes the local variable, which looks like what Pablo originally
> intended.
Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
Sorry for this, I wonder why gcc didn't catch up this here.
@David, you can integrate this into your net-next tree.
Thanks for fixing up this Arnd.
^ permalink raw reply
* Re: [net-next PATCH V1 3/3] net: warn on napi_alloc_skb being called in wrong context
From: Jesper Dangaard Brouer @ 2016-05-09 20:03 UTC (permalink / raw)
To: Alexander Duyck
Cc: Netdev, David S. Miller, Saeed Mahameed, Or Gerlitz,
Eugenia Emantayev, brouer
In-Reply-To: <CAKgT0UcJyxDvY+r6qsXSCQ4SjE2VH4NPGOrG9F7Ame50cQQpKA@mail.gmail.com>
On Mon, 9 May 2016 09:33:25 -0700
Alexander Duyck <alexander.duyck@gmail.com> wrote:
> On Mon, May 9, 2016 at 6:44 AM, Jesper Dangaard Brouer
> <brouer@redhat.com> wrote:
> > It have always been required to call napi_alloc_skb from NAPI/softirq
> > context, which implies running with local_bh_disable'ed. Thus, this
> > code path should already be well tested. But recent SKB bulk changes
> > introduced will make this more volatile and bugs more subtle, if this
> > is violated.
>
> It hasn't been required to be in the softirq routine, bottom halves
> could have been disabled. I used the NAPI parameter to try and
> enforce a strong correlation between the two but I didn't make it a
> hard requirement. It isn't until you try to do the recycling that it
> becomes more of a requirement because then it is tied into the bulk
> free.
>
> The bulk free routine had a stronger requirement for NAPI because it
> needed to have the __kfree_skb_flush routine called to finally free
> the skbuffs that were still hanging off the percpu skbuff structure.
>
> > To catch any driver violating this add a loud WARN_ON.
> >
> > Performance wise, I do worry about adding this runtime check code into
> > the hotpath, of this highly optimized function call. I've
> > micro-benchmarked it with both IP-forwarding and local UDP delivery,
> > and didn't see any regressions. It does adds extra code size (icache).
> >
> > add/remove: 0/0 grow/shrink: 1/0 up/down: 43/0 (43)
> > function old new delta
> > __napi_alloc_skb 461 504 +43
> >
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > ---
> > net/core/skbuff.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index e85f1065b263..99addbf66f2e 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -498,6 +498,9 @@ struct sk_buff *__napi_alloc_skb(struct napi_struct *napi, unsigned int len,
> > struct sk_buff *skb;
> > void *data;
> >
> > + /* Catch drivers violating, not having local BH disabled */
> > + WARN_ON(!in_softirq());
> > +
>
> I'm pretty sure this still lets you get away with just disabling
> bottom halves. I think what you would want is in_serving_softirq().
No, we want to allow this being call with just disabled bottom halves.
Thus, we have to use in_softirq() and not in_serving_softirq().
> > len += NET_SKB_PAD + NET_IP_ALIGN;
> >
> > if (unlikely((len > SKB_WITH_OVERHEAD(PAGE_SIZE)) ||
> >
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [net-next PATCH V1 2/3] mlx4: use napi_alloc_skb API to get SKB bulk allocations
From: Jesper Dangaard Brouer @ 2016-05-09 20:05 UTC (permalink / raw)
To: Alexander Duyck
Cc: Netdev, David S. Miller, Saeed Mahameed, Or Gerlitz,
Eugenia Emantayev, brouer
In-Reply-To: <CAKgT0UdcAzpYnqvv8HkMaPoPxLPj5-iJpwzdJWVKX9DfYJoYyg@mail.gmail.com>
On Mon, 9 May 2016 09:47:08 -0700
Alexander Duyck <alexander.duyck@gmail.com> wrote:
> On Mon, May 9, 2016 at 6:44 AM, Jesper Dangaard Brouer
> <brouer@redhat.com> wrote:
> > Activate the bulk alloc API, simply by changing mlx4 from using
> > netdev_alloc_skb() to using napi_alloc_skb().
>
> This patch is just enabling the napi_alloc_skb call. You don't need
> to call out that it is enabling bulk allocations. This patch could
> stand on its own without needing to make reference to the bulk
> allocation API because there is enough of a gain from napi_alloc_skb
> replacing netdev_alloc_skb.
Okay, I see you point.
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > ---
> > drivers/net/ethernet/mellanox/mlx4/en_rx.c | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> > index 8ef6875b6cf9..84fd6db5a176 100644
> > --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> > +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> > @@ -577,14 +577,15 @@ fail:
> > static struct sk_buff *mlx4_en_rx_skb(struct mlx4_en_priv *priv,
> > struct mlx4_en_rx_desc *rx_desc,
> > struct mlx4_en_rx_alloc *frags,
> > - unsigned int length)
> > + unsigned int length,
> > + struct napi_struct *napi)
>
> Instead of passing the NAPI structure you could just pass the
> mlx4_en_cq pointer to be used by the NAPI alloc function. In addition
> you might try adding the new parameter before length since that way
> the pointers are in one block followed by integers in a tapering
> length order.
Okay, will adjust.
> > {
> > struct sk_buff *skb;
> > void *va;
> > int used_frags;
> > dma_addr_t dma;
> >
> > - skb = netdev_alloc_skb(priv->dev, SMALL_PACKET_SIZE + NET_IP_ALIGN);
> > + skb = napi_alloc_skb(napi, SMALL_PACKET_SIZE + NET_IP_ALIGN);
>
> The NET_IP_ALIGN is redundant as napi_alloc_skb already takes are of
> adding that and NET_SKB_PAD.
Thanks for catching this.
> > if (!skb) {
> > en_dbg(RX_ERR, priv, "Failed allocating skb\n");
> > return NULL;
> > @@ -932,7 +933,7 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
> > }
> >
> > /* GRO not possible, complete processing here */
> > - skb = mlx4_en_rx_skb(priv, rx_desc, frags, length);
> > + skb = mlx4_en_rx_skb(priv, rx_desc, frags, length, &cq->napi);
> > if (!skb) {
> > ring->dropped++;
> > goto next;
> >
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH] netfilter: conntrack: remove uninitialized shadow variable
From: Arnd Bergmann @ 2016-05-09 20:12 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Patrick McHardy, Jozsef Kadlecsik, David S. Miller,
Florian Westphal, Daniel Borkmann, Sasha Levin, netfilter-devel,
coreteam, netdev, linux-kernel
In-Reply-To: <20160509200117.GA6957@salvia>
On Monday 09 May 2016 22:01:17 Pablo Neira Ayuso wrote:
> On Mon, May 09, 2016 at 09:47:23PM +0200, Arnd Bergmann wrote:
> > A recent commit introduced an unconditional use of an uninitialized
> > variable, as reported in this gcc warning:
> >
> > net/netfilter/nf_conntrack_core.c: In function '__nf_conntrack_confirm':
> > net/netfilter/nf_conntrack_core.c:632:33: error: 'ctinfo' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > bytes = atomic64_read(&counter[CTINFO2DIR(ctinfo)].bytes);
> > ^
> > net/netfilter/nf_conntrack_core.c:628:26: note: 'ctinfo' was declared here
> > enum ip_conntrack_info ctinfo;
> >
> > The problem is that a local variable shadows the function parameter.
> > This removes the local variable, which looks like what Pablo originally
> > intended.
>
> Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
>
> Sorry for this, I wonder why gcc didn't catch up this here.
>
> @David, you can integrate this into your net-next tree.
>
> Thanks for fixing up this Arnd.
By default, an allmodconfig build will hide these warnings because of
excessive false positives from CONFIG_CC_OPTIMIZE_FOR_SIZE. I've
tried twice to get a patch merged that disables CONFIG_CC_OPTIMIZE_FOR_SIZE
in allmodconfig so we get better warnings, but that patch unfortunately
got ignored.
Arnd
^ permalink raw reply
* bnx2x in 4.6rc7 with FW 7.13.1.0 not present
From: Elliott, Robert (Persistent Memory) @ 2016-05-09 20:19 UTC (permalink / raw)
To: yuval.mintz@qlogic.com, Ariel.Elior@qlogic.com,
davem@davemloft.net
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Upgrading a system from kernel 4.2 to 4.6rc7, there is an extra 2 minute
delay while booting due to these problems:
[ 47.977221] bnx2x 0000:04:00.1: Direct firmware load for bnx2x/bnx2x-e2-7.13.1.0.fw failed with error -2
[ 48.029997] bnx2x 0000:04:00.1: Falling back to user helper
[** ] A start job is running for Network Manager Wait Online (1min 27s / no limit)
[ 108.064132] bnx2x: [bnx2x_init_firmware:13472(eno50)]Can't load firmware file bnx2x/bnx2x-e2-7.13.1.0.fw
[ 108.119951] bnx2x: [bnx2x_func_hw_init:5785(eno50)]Error loading firmware
[ 108.156522] bnx2x: [bnx2x_nic_load:2727(eno50)]HW init failed, aborting
[* ] A start job is running for Network Manager Wait Online (1min 28s / no limit)
These are the versions in /lib/firmware/bnx2x (a RHEL installation):
bnx2x-e2-6.2.9.0.fw bnx2x-e1-7.8.19.0.fw bnx2x-e1h-7.8.2.0.fw
bnx2x-e1h-6.2.9.0.fw bnx2x-e1-7.8.2.0.fw bnx2x-e2-6.0.34.0.fw
bnx2x-e1-6.2.9.0.fw bnx2x-e1h-6.0.34.0.fw bnx2x-e2-6.2.5.0.fw
bnx2x-e1-6.0.34.0.fw bnx2x-e1h-6.2.5.0.fw bnx2x-e2-7.0.20.0.fw
bnx2x-e1-6.2.5.0.fw bnx2x-e1h-7.0.20.0.fw bnx2x-e2-7.0.23.0.fw
bnx2x-e1-7.0.20.0.fw bnx2x-e1h-7.0.23.0.fw bnx2x-e2-7.0.29.0.fw
bnx2x-e1-7.0.23.0.fw bnx2x-e1h-7.0.29.0.fw bnx2x-e2-7.10.51.0.fw
bnx2x-e1-7.0.29.0.fw bnx2x-e1h-7.10.51.0.fw bnx2x-e2-7.12.30.0.fw
bnx2x-e1-7.10.51.0.fw bnx2x-e1h-7.12.30.0.fw bnx2x-e2-7.2.16.0.fw
bnx2x-e1-7.12.30.0.fw bnx2x-e1h-7.2.16.0.fw bnx2x-e2-7.2.51.0.fw
bnx2x-e1-7.2.16.0.fw bnx2x-e1h-7.2.51.0.fw bnx2x-e2-7.8.17.0.fw
bnx2x-e1-7.2.51.0.fw bnx2x-e1h-7.8.17.0.fw bnx2x-e2-7.8.19.0.fw
bnx2x-e1-7.8.17.0.fw bnx2x-e1h-7.8.19.0.fw bnx2x-e2-7.8.2.0.fw
Reverting 5e091e7a "bnx2x: Utilize FW 7.13.1.0" avoids the problem.
I assume getting 7.13.1.0 from the linux-firmware project will also do so.
Could the driver fall back to an older firmware version more gracefully?
4.6rc7
======
[ 0.000000] Linux version 4.6.0-rc7+ ... (gcc version 4.8.5 20150623 (Red Hat 4.8.5-4) (GCC) ) #7 SMP Mon May 9 14:22:53 EDT 2016
[ 0.000000] Command line: BOOT_IMAGE=/vmlinuz-4.6.0-rc7+ root=/dev/mapper/rhel-root ro crashkernel=auto rd.lvm.lv=rhel/root rd.lvm.lv=rhel/swap rhgb quiet efi=debug ... ignore_loglevel console=ttyS1,115200 console=tty0 LANG=en_US.UTF-8
...
[ 16.143816] bnx2x: QLogic 5771x/578xx 10/20-Gigabit Ethernet Driver bnx2x 1.712.30-0 (2014/02/10)
[ 16.144780] bnx2x 0000:04:00.0: msix capability found
[ 16.144995] bnx2x 0000:04:00.0: part number 0-0-0-0
[ 16.237686] bnx2x 0000:04:00.1: msix capability found
[ 16.237859] bnx2x 0000:04:00.1: part number 0-0-0-0
[ 16.525718] bnx2x 0000:04:00.1 eno50: renamed from eth2
[ 16.648816] bnx2x 0000:04:00.0 eno49: renamed from eth1
...
[ 47.333227] IPv6: ADDRCONF(NETDEV_UP): eno2: link is not ready
[ 47.498841] IPv6: ADDRCONF(NETDEV_UP): eno2: link is not ready
[ 47.530225] IPv6: ADDRCONF(NETDEV_UP): eno3: link is not ready
[ 47.680716] IPv6: ADDRCONF(NETDEV_UP): eno3: link is not ready
[ 47.714908] IPv6: ADDRCONF(NETDEV_UP): eno1: link is not ready
[ 47.874737] IPv6: ADDRCONF(NETDEV_UP): eno1: link is not ready
[ 47.907457] IPv6: ADDRCONF(NETDEV_UP): eno50: link is not ready
[ 47.977221] bnx2x 0000:04:00.1: Direct firmware load for bnx2x/bnx2x-e2-7.13.1.0.fw failed with error -2
[ 48.029997] bnx2x 0000:04:00.1: Falling back to user helper
[ 51.104604] tg3 0000:02:00.1 eno2: Link is up at 1000 Mbps, full duplex
[ 51.142403] tg3 0000:02:00.1 eno2: Flow control is off for TX and off for RX
[ 51.180648] tg3 0000:02:00.1 eno2: EEE is disabled
[** ] A start job is running for Network Manager Wait Online (1min 27s / no limit)
[ 108.064132] bnx2x: [bnx2x_init_firmware:13472(eno50)]Can't load firmware file bnx2x/bnx2x-e2-7.13.1.0.fw
[ 108.119951] bnx2x: [bnx2x_func_hw_init:5785(eno50)]Error loading firmware
[ 108.156522] bnx2x: [bnx2x_nic_load:2727(eno50)]HW init failed, aborting
[* ] A start job is running for Network Manager Wait Online (1min 28s / no limit)
[ 108.435477] IPv6: ADDRCONF(NETDEV_CHANGE): eno2: link becomes ready
[ 108.475065] IPv6: ADDRCONF(NETDEV_UP): eno49: link is not ready
[ 108.556168] bnx2x 0000:04:00.0: Direct firmware load for bnx2x/bnx2x-e2-7.13.1.0.fw failed with error -2
[ 108.608229] bnx2x 0000:04:00.0: Falling back to user helper
[ 169.337933] IPv6: ADDRCONF(NETDEV_UP): eno1: link is not ready
[ 169.338749] IPv6: ADDRCONF(NETDEV_UP): eno50: link is not ready
[ 169.339772] IPv6: ADDRCONF(NETDEV_UP): eno49: link is not ready
[ 169.340630] IPv6: ADDRCONF(NETDEV_UP): eno4: link is not ready
Starting WPA Supplicant daemon...
[ OK ] Started WPA Supplicant daemon.
Starting Network Manager Script Dispatcher Service...
[ OK ] Started Network Manager Script Dispatcher Service.
[ OK ] Started Network Manager Wait Online.
Starting LSB: Bring up/down networking...
[FAILED] Failed to start LSB: Bring up/down networking.
See 'systemctl status network.service' for details.
[ OK ] Reached target Network.
Starting Logout off all iSCSI sessions on shutdown...
Starting Enable periodic update of entitlement certificates....
Starting Dynamic System Tuning Daemon...
[ OK ] Started CUPS Printing Service.
Starting CUPS Printing Service...
Starting Notify NFS peers of a restart...
[ OK ] Started OpenSSH server daemon.
Starting OpenSSH server daemon...
Starting Postfix Mail Transport Agent...
[ OK ] Reached target Network is Online.
[ OK ] Started Logout off all iSCSI sessions on shutdown.
[ OK ] Started Enable periodic update of entitlement certificates..
[ OK ] Started Notify NFS peers of a restart.
[ OK ] Started Dynamic System Tuning Daemon.
[ OK ] Reached target Remote File Systems (Pre).
[ OK ] Reached target Remote File Systems.
Starting Virtualization daemon...
Starting Permit User Sessions...
Starting LSB: Starts the Spacewalk Daemon...
Starting Crash recovery kernel arming...
Starting Availability of block devices...
[ OK ] Started Virtualization daemon.
[ OK ] Started Permit User Sessions.
[ OK ] Started LSB: Starts the Spacewalk Daemon.
[ OK ] Started Availability of block devices.
Starting Wait for Plymouth Boot Screen to Quit...
[ OK ] Started Command Scheduler.
Starting Command Scheduler...
[ OK ] Started Job spooling tools.
Starting Job spooling tools...
Starting GNOME Display Manager...
[ 176.171405] tun: Universal TUN/TAP device driver, 1.6
[ 176.171406] tun: (C) 1999-2004 Max Krasnyansky <maxk@qualcomm.com>
[ OK ] Started Postfix Mail Transport Agent.
[ OK ] Started GNOME Display Manager.
[ 176.747016] virbr0: port 1(virbr0-nic) entered blocking state
[ 176.778078] virbr0: port 1(virbr0-nic) entered disabled state
[ 176.808991] device virbr0-nic entered promiscuous mode
[ 177.007764] virbr0: port 1(virbr0-nic) entered blocking state
[ 177.011281] virbr0: port 1(virbr0-nic) entered listening state
[ 177.046773] virbr0: port 1(virbr0-nic) entered disabled state
Red Hat Enterprise Linux Server 7.2 (Maipo)
Kernel 4.6.0-rc7+ on an x86_64
s71 login:
4.2
===
[ 0.000000] Linux version 4.2.0-380.13.hpoj.x86_64 (root@blrh6u2x64005) (gcc version 4.8.3 20140911 (Red Hat 4.8.3-9) (GCC) ) #1 SMP Fri Mar 4 13:19:33 CST 2016
[ 0.000000] Command line: BOOT_IMAGE=/vmlinuz-4.2.0-380.13.hpoj.x86_64 root=/dev/mapper/rhel-root ro crashkernel=auto rd.lvm.lv=rhel/root rd.lvm.lv=rhel/swap rhgb quiet efi=debug ... ignore_loglevel console=ttyS1,115200 console=tty0
...
[ 46.888499] IPv6: ADDRCONF(NETDEV_UP): eno2: link is not ready
[ 46.889525] IPv6: ADDRCONF(NETDEV_UP): eno50: link is not ready
Starting Network Manager Wait Online...
[ 47.420000] bnx2x 0000:04:00.1 eno50: using MSI-X IRQs: sp 108 fp[0] 110 ... fp[7] 117
[ 48.118821] IPv6: ADDRCONF(NETDEV_UP): eno50: link is not ready
[ 48.153042] IPv6: ADDRCONF(NETDEV_UP): eno3: link is not ready
[ 48.324725] IPv6: ADDRCONF(NETDEV_UP): eno3: link is not ready
[ 48.360099] IPv6: ADDRCONF(NETDEV_UP): eno1: link is not ready
[ 48.530094] IPv6: ADDRCONF(NETDEV_UP): eno1: link is not ready
[ 48.564963] IPv6: ADDRCONF(NETDEV_UP): eno49: link is not ready
[ 49.112598] bnx2x 0000:04:00.0 eno49: using MSI-X IRQs: sp 98 fp[0] 100 ... fp[7] 107
[ 49.716845] IPv6: ADDRCONF(NETDEV_UP): eno49: link is not ready
[ 49.752453] IPv6: ADDRCONF(NETDEV_UP): eno4: link is not ready
[ 49.918571] IPv6: ADDRCONF(NETDEV_UP): eno4: link is not ready
[ 49.957314] IPv6: ADDRCONF(NETDEV_UP): eno2: link is not ready
[ 49.989835] IPv6: ADDRCONF(NETDEV_UP): eno50: link is not ready
[ 50.022918] IPv6: ADDRCONF(NETDEV_UP): eno3: link is not ready
[ 50.055375] IPv6: ADDRCONF(NETDEV_UP): eno1: link is not ready
[ 50.087663] IPv6: ADDRCONF(NETDEV_UP): eno49: link is not ready
[ 50.119932] IPv6: ADDRCONF(NETDEV_UP): eno4: link is not ready
Starting WPA Supplicant daemon...
[ OK ] Started WPA Supplicant daemon.
[ 50.401735] tg3 0000:02:00.1 eno2: Link is up at 1000 Mbps, full duplex
[ 50.439124] tg3 0000:02:00.1 eno2: Flow control is off for TX and off for RX
[ 50.476727] tg3 0000:02:00.1 eno2: EEE is disabled
[ 50.502943] IPv6: ADDRCONF(NETDEV_CHANGE): eno2: link becomes ready
[ OK ] Started Network Manager Wait Online.
Starting LSB: Bring up/down networking...
Starting Network Manager Script Dispatcher Service...
[ OK ] Started Network Manager Script Dispatcher Service.
[FAILED] Failed to start LSB: Bring up/down networking.
See 'systemctl status network.service' for details.
[ OK ] Reached target Network.
Starting Notify NFS peers of a restart...
[ OK ] Reached target Network is Online.
Starting Dynamic System Tuning Daemon...
Starting Enable periodic update of entitlement certificates....
[ OK ] Started CUPS Printing Service.
Starting CUPS Printing Service...
Starting Logout off all iSCSI sessions on shutdown...
Starting Postfix Mail Transport Agent...
[ OK ] Started OpenSSH server daemon.
Starting OpenSSH server daemon...
[ OK ] Started Notify NFS peers of a restart.
[ OK ] Started Enable periodic update of entitlement certificates..
[ OK ] Started Logout off all iSCSI sessions on shutdown.
Starting Availability of block devices...
[ OK ] Reached target Remote File Systems (Pre).
[ OK ] Reached target Remote File Systems.
Starting Crash recovery kernel arming...
Starting Virtualization daemon...
Starting Permit User Sessions...
Starting LSB: Starts the Spacewalk Daemon...
[ OK ] Started Availability of block devices.
[ OK ] Started Permit User Sessions.
[ OK ] Started LSB: Starts the Spacewalk Daemon.
[ OK ] Started Dynamic System Tuning Daemon.
Starting GNOME Display Manager...
[ OK ] Started Command Scheduler.
Starting Command Scheduler...
[ OK ] Started Job spooling tools.
Starting Job spooling tools...
Starting Wait for Plymouth Boot Screen to Quit...
[ OK ] Started Virtualization daemon.
[ OK ] Started GNOME Display Manager.
[ 59.203229] tun: Universal TUN/TAP device driver, 1.6
[ 59.232283] tun: (C) 1999-2004 Max Krasnyansky <maxk@qualcomm.com>
[ 59.624817] virbr0: port 1(virbr0-nic) entered blocking state
[ 59.656216] virbr0: port 1(virbr0-nic) entered disabled state
[ 59.687578] device virbr0-nic entered promiscuous mode
[ 59.879160] virbr0: port 1(virbr0-nic) entered blocking state
[ 59.882383] virbr0: port 1(virbr0-nic) entered listening state
[ 60.865748] bnx2x 0000:04:00.0 eno49: using MSI-X IRQs: sp 98 fp[0] 100 ... fp[7] 107
[ 61.891547] virbr0: port 1(virbr0-nic) entered learning state
[ 62.217602] bnx2x 0000:04:00.1 eno50: using MSI-X IRQs: sp 108 fp[0] 110 ... fp[7] 117
[ 62.905227] virbr0: port 1(virbr0-nic) entered disabled state
Red Hat Enterprise Linux Server 7.2 (Maipo)
Kernel 4.6.0-rc7+ on an x86_64
s71 login:
---
Robert Elliott, HPE Persistent Memory
^ permalink raw reply
* Re: [PATCH net-next V3 00/10] cls_flower hardware offload support
From: Or Gerlitz @ 2016-05-09 20:31 UTC (permalink / raw)
To: David Miller
Cc: Amir Vadai, Linux Netdev List, John Fastabend, Saeed Mahameed,
Jiri Pirko
In-Reply-To: <20160310.162435.690159053489962918.davem@davemloft.net>
On Fri, Mar 11, 2016 at 12:24 AM, David Miller <davem@davemloft.net> wrote:
> From: Amir Vadai <amir@vadai.me>
> Date: Tue, 8 Mar 2016 12:42:28 +0200
>> This patchset introduces cls_flower hardware offload support over ConnectX-4
>> driver, more hardware vendors are welcome to use it too.
> Series applied, thanks for retaining detailed change history in this series
> header posting.
Hi Dave,
Amir was working lately on a follow up series which adds support to HW
statistics (packet/bytes and lastuse) for offloaded TC flower flows.
The series contains patches to TC/flower and to mlx5.
Wanted to check with you if such a submission can take place in
parallel with another net-next mlx5 only submission which is planned
by the driver team.
Thanks,
Or.
^ permalink raw reply
* Re: [net-next PATCH V1 1/3] net: bulk alloc and reuse of SKBs in NAPI context
From: Alexander Duyck @ 2016-05-09 20:46 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Netdev, David S. Miller, Saeed Mahameed, Or Gerlitz,
Eugenia Emantayev
In-Reply-To: <20160509215956.19ec1c10@redhat.com>
On Mon, May 9, 2016 at 12:59 PM, Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
> On Mon, 9 May 2016 09:20:41 -0700
> Alexander Duyck <alexander.duyck@gmail.com> wrote:
>
>> On Mon, May 9, 2016 at 6:44 AM, Jesper Dangaard Brouer
>> <brouer@redhat.com> wrote:
>> > This patch introduce bulk alloc of SKBs and allow reuse of SKBs
>> > free'ed in same softirq cycle. SKBs are normally free'ed during TX
>> > completion, but most high speed drivers also cleanup TX ring during
>> > NAPI RX poll cycle. Thus, if using napi_consume_skb/__kfree_skb_defer,
>> > SKBs will be avail in the napi_alloc_cache->skb_cache.
>> >
>> > If no SKBs are avail for reuse, then only bulk alloc 8 SKBs, to limit
>> > the potential overshooting unused SKBs needed to free'ed when NAPI
>> > cycle ends (flushed in net_rx_action via __kfree_skb_flush()).
>> >
>> > Generalize ___build_skb() to allow passing it a preallocated SKB.
>> >
>> > I've previously demonstrated a 1% speedup for IPv4 forwarding, when
>> > used on the ixgbe driver. If SKB alloc and free happens on different
>> > CPUs (like in RPS use-case) the performance benefit is expected to
>> > increase.
>>
>> Really I am not sure this patch series is worth the effort. For
>> freeing buffers in Tx it was an obvious win. But adding all this
>> complexity for a 1% gain just doesn't seen worth the effort.
>
> I still think it is worth it, because: 1) it enables recycling, which
> is more likely for real-life traffic (e.g. some TCP ACKs gets TX DMA
> completion cleanup, and RX can re-use these), and 2) because bulk alloc
> and bulk free gets "coupled" (mostly relevant when doing cross CPU).
I disagree. While there will be some ACKs it will likely be
significantly less then the number of packets received, especially
when you take GRO into account.
>
>> > All drivers using the napi_alloc_skb() call will benefit from
>> > this change automatically.
>>
>> Yes, but a 1% improvement is essentially just noise. I'd say we need
>> to show a better gain or be able to more precisely show that this is a
>> definite gain and not just a test fluctuation resulting in the
>> improvement. For all I know all of the gain could be due to a
>> function shifting around so that some loop is now 16 byte aligned.
>>
>> > Joint work with Alexander Duyck.
>> >
>> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
>> > Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
>>
>> The fact that this is still using my redhat.com address says volumes.
>> If I recall I think we were working on this code around 9 months ago
>> and had similar issues with it showing either negative performance or
>> no gain. My advice then was to push the bulk free code and try to
>> find a way to fix the bulk allocation. If we are still at this state
>> for bulk allocation then maybe we need to drop the bulk allocation and
>> start looking at other avenues to pursue.
>
> I'm sure there is a gain here. Sure I can spend 2-3 weeks coming up
> with a benchmark that show a bigger gain, but is it time well spend?
>
> My quick benchmarking with mlx4 actually showed 2.7% improvement, for
> local UDP socket delivery. And only 0.6% on IP-forwarding. If I find
> some bidirectional traffic then the benefit should be higher due to
> recycling kicking in.
Is that with or without patch 2 being a part of the set? This patch
should be able to prove itself as a stand-alone patch. I'd say go
ahead and submit patch 2 as a standalone and then we can start working
on collecting numbers on the mlx4. Also are you running your test
with GRO disabled? If so you might try enabling it on the mlx4
because that changes the skbuff allocation which would impact the
behavior for the device.
Try testing with TCP_RR instead and watch the CPU utilization. I'm
suspecting allocating 8 and freeing 7 buffers for every 1 buffer
received will blow any gains right out of the water. Also try it with
a mix of traffic. So have one NIC doing TCP_RR while another is doing
a stream test. You are stuffing 7 buffers onto a queue that were were
using to perform bulk freeing. How much of a penalty do you take if
you are now limited on how many you can bulk free because somebody
left a stray 7 packets sitting on the queue?
Also please stop focusing on IP-forwarding. There are some use cases
for it out there, but the vast majority of people don't care that much
about it. It is just one data point out of many. Try running a few
TCP based applications and see if you can notice any difference. It
is okay to focus on routing numbers when you are doing work that will
only impact routing or has next to no likelihood of introducing a
regression, but the fact is the bulk alloc has a strong likelihood of
having effects on other parts of the network stack since you are
pulling in more buffers then you actually need.
> We did do as you recommended, and the bulk free code first. I'm
> just getting the last pieces pushed. I didn't see any negative
> performance in my testing.
Not seeing any negative performance is not the same as seeing gains.
You are introducing complexity, there needs to be something to show
for it. Otherwise you are just making the code harder to maintain for
no good reason.
> As you also know, tuning the SLUB system will give higher performance,
> easily. In the future, I'm planning to get some auto-tuning into the
> SLUB allocator. I've already discussed this with Christiph Lameter, at
> MM-summit, see presentation[1] slides 4 and 5.
We aren't discussing tuning parameters. We are discussing this patch.
If you want to argue that with certain tuning parameters this shows
more performance then bring the numbers, but don't try to bias things.
If you have to tune the system in some way that nobody will there is
probably no point in submitting the patch because nobody will use it
that way.
You also didn't test for low latency. As I said this is coming at a
high expense to other use cases, but your use case only shows a 1%
improvement. In my opinion it is not worth adding the complexity if
we cannot show much of a gain. If you want to win me over we need to
be able to justify the complexity. Right now I am not seeing it. In
addition patch 3 illustrates the added complexity you are bringing to
all this because you are having to add limitations to napi_alloc_skb
because you are trying to make it work with recycling which
historically has never been a win because there are too many traffic
patterns where recycling cannot occur.
> [1] http://people.netfilter.org/hawk/presentations/MM-summit2016/slab_mm_summit2016.odp
>
>> > ---
>> > net/core/skbuff.c | 71
>> > ++++++++++++++++++++++++++++++++++------------------- 1 file
>> > changed, 45 insertions(+), 26 deletions(-)
>> >
>> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> > index 5586be93632f..e85f1065b263 100644
>> > --- a/net/core/skbuff.c
>> > +++ b/net/core/skbuff.c
>> > @@ -281,32 +281,14 @@ nodata:
>> > }
>> > EXPORT_SYMBOL(__alloc_skb);
>> >
>> > -/**
>> > - * __build_skb - build a network buffer
>> > - * @data: data buffer provided by caller
>> > - * @frag_size: size of data, or 0 if head was kmalloced
>> > - *
>> > - * Allocate a new &sk_buff. Caller provides space holding head and
>> > - * skb_shared_info. @data must have been allocated by kmalloc()
>> > only if
>> > - * @frag_size is 0, otherwise data should come from the page
>> > allocator
>> > - * or vmalloc()
>> > - * The return is the new skb buffer.
>> > - * On a failure the return is %NULL, and @data is not freed.
>> > - * Notes :
>> > - * Before IO, driver allocates only data buffer where NIC put
>> > incoming frame
>> > - * Driver should add room at head (NET_SKB_PAD) and
>> > - * MUST add room at tail (SKB_DATA_ALIGN(skb_shared_info))
>> > - * After IO, driver calls build_skb(), to allocate sk_buff and
>> > populate it
>> > - * before giving packet to stack.
>> > - * RX rings only contains data buffers, not full skbs.
>> > - */
>> > -struct sk_buff *__build_skb(void *data, unsigned int frag_size)
>> > +/* Allows skb being (pre)allocated by caller */
>> > +static inline
>> > +struct sk_buff *___build_skb(void *data, unsigned int frag_size,
>> > + struct sk_buff *skb)
>> > {
>> > struct skb_shared_info *shinfo;
>> > - struct sk_buff *skb;
>> > unsigned int size = frag_size ? : ksize(data);
>> >
>> > - skb = kmem_cache_alloc(skbuff_head_cache, GFP_ATOMIC);
>> > if (!skb)
>> > return NULL;
>> >
>> > @@ -331,6 +313,34 @@ struct sk_buff *__build_skb(void *data,
>> > unsigned int frag_size) return skb;
>> > }
>> >
>> > +/**
>> > + * __build_skb - build a network buffer
>> > + * @data: data buffer provided by caller
>> > + * @frag_size: size of data, or 0 if head was kmalloced
>> > + *
>> > + * Allocate a new &sk_buff. Caller provides space holding head and
>> > + * skb_shared_info. @data must have been allocated by kmalloc()
>> > only if
>> > + * @frag_size is 0, otherwise data should come from the page
>> > allocator
>> > + * or vmalloc()
>> > + * The return is the new skb buffer.
>> > + * On a failure the return is %NULL, and @data is not freed.
>> > + * Notes :
>> > + * Before IO, driver allocates only data buffer where NIC put
>> > incoming frame
>> > + * Driver should add room at head (NET_SKB_PAD) and
>> > + * MUST add room at tail (SKB_DATA_ALIGN(skb_shared_info))
>> > + * After IO, driver calls build_skb(), to allocate sk_buff and
>> > populate it
>> > + * before giving packet to stack.
>> > + * RX rings only contains data buffers, not full skbs.
>> > + */
>> > +struct sk_buff *__build_skb(void *data, unsigned int frag_size)
>> > +{
>> > + struct sk_buff *skb;
>> > + unsigned int size = frag_size ? : ksize(data);
>> > +
>> > + skb = kmem_cache_alloc(skbuff_head_cache, GFP_ATOMIC);
>> > + return ___build_skb(data, size, skb);
>> > +}
>> > +
>> > /* build_skb() is wrapper over __build_skb(), that specifically
>> > * takes care of skb->head and skb->pfmemalloc
>> > * This means that if @frag_size is not zero, then @data must be
>> > backed
>>
>> If we can avoid having to break up build_skb into more functions that
>> would be preferred. I realize I probably wrote this code in order to
>> enable the bulk allocation approach, but really I don't want to add
>> more complexity unless we can show a strong gain which we haven't been
>> able to demonstrate.
>
> You do notice that ___build_skb gets inlined, thus there is not
> performance loss. And this change was explicitly requested last time
> this patch was reviewed. And I think this variant is much less
> intrusive.
I didn't notice the inline, so it means we are bloating the code a bit
instead of adding more function calls. I guess that is a better trade
off, but still not the most desirable thing to do.
>
>> > @@ -490,8 +500,8 @@ struct sk_buff *__napi_alloc_skb(struct
>> > napi_struct *napi, unsigned int len,
>> >
>> > len += NET_SKB_PAD + NET_IP_ALIGN;
>> >
>> > - if ((len > SKB_WITH_OVERHEAD(PAGE_SIZE)) ||
>> > - (gfp_mask & (__GFP_DIRECT_RECLAIM | GFP_DMA))) {
>> > + if (unlikely((len > SKB_WITH_OVERHEAD(PAGE_SIZE)) ||
>> > + (gfp_mask & (__GFP_DIRECT_RECLAIM |
>> > GFP_DMA)))) { skb = __alloc_skb(len, gfp_mask, SKB_ALLOC_RX,
>> > NUMA_NO_NODE); if (!skb)
>> > goto skb_fail;
>>
>> Does this unlikely really make a difference? If so you might want to
>> move it into a patch all on its own.
>
> I can place it in a patch of it's own. I just noticed the compiler
> layed out the code wrongly compared to the normal use-case.
What version of gcc are you seeing this with? From what I can tell on
the 4.8.5 version that came with my RHEL 7.2 build I am seeing the
proper setup with 2 compares and 2 jumps to the remote code.
>> > @@ -508,11 +518,20 @@ struct sk_buff *__napi_alloc_skb(struct
>> > napi_struct *napi, unsigned int len, if (unlikely(!data))
>> > return NULL;
>> >
>> > - skb = __build_skb(data, len);
>> > - if (unlikely(!skb)) {
>> > +#define BULK_ALLOC_SIZE 8
>> > + if (!nc->skb_count) {
>> > + nc->skb_count =
>> > kmem_cache_alloc_bulk(skbuff_head_cache,
>> > + gfp_mask,
>> > BULK_ALLOC_SIZE,
>> > +
>> > nc->skb_cache);
>> > + }
>> > + if (likely(nc->skb_count)) {
>> > + skb = (struct sk_buff
>> > *)nc->skb_cache[--nc->skb_count];
>> > + } else {
>> > + /* alloc bulk failed */
>>
>> So did you try doing a low latency socket test with this patch? I'm
>> guessing not as I am certain there is a negative impact from having to
>> allocate 8 buffers, and then free back 7 every time you call the NAPI
>> polling routine with just one buffer in the ring.
>
> There is a very high probability that pulling out 8 objects, and
> returning 7 object, will have the same cost of alloc and free of a
> single object. This is due to how the SLUB allocator's per CPU
> allocator works.
I very much doubt that. You are still having to move around many more
objects then you really should be and dragging in 8 cache lines as you
have to walk the percpu freelist to pull the entries and then push
them back in.
> Notice I said "high probability". I have adjusted the slab bulk APIs,
> such that we can extend them to bulk return "upto" a given number of
> objects. Then the SLUB allocator can remove the "high probability"
> part, and make sure only to return object on the per CPU slab-page,
> thus guaranteeing no cmpxchg_double calls, and only
> local_irq_disable/enable cycles, which is actually faster than the
> normal fastpath local cmpxchg_double (non-atomic variant).
On your system. There is no guarantee that is the case across all
CPUs supported by the x86 architecture.
Also I can think of scenarios where your code would become very
expensive. The fact is you are pushing an additional 7 objects into
the L1 cache each time you do one of these bulk allocations. The
question you have to ask yourself is what is it you are evicting when
you do it. There are scenerios where you could be evicting critical
data back out to L2 which will negatively impact your performance. A
1% gain 99% of the time is kind of useless when there is a risk of a
significant performance hit for that remaining 1% of cases.
^ permalink raw reply
* [iproute PATCH] ss: Remove unused argument from kill_inet_sock
From: Subash Abhinov Kasiviswanathan @ 2016-05-09 20:54 UTC (permalink / raw)
To: lorenzo, netdev, stephen; +Cc: Subash Abhinov Kasiviswanathan
addr is not used here.
Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
---
misc/ss.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/misc/ss.c b/misc/ss.c
index deefc96..5cda728 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -2232,8 +2232,7 @@ struct inet_diag_arg {
struct rtnl_handle *rth;
};
-static int kill_inet_sock(const struct sockaddr_nl *addr,
- struct nlmsghdr *h, void *arg)
+static int kill_inet_sock(struct nlmsghdr *h, void *arg)
{
struct inet_diag_msg *d = NLMSG_DATA(h);
struct inet_diag_arg *diag_arg = arg;
@@ -2260,7 +2259,7 @@ static int show_one_inet_sock(const struct sockaddr_nl *addr,
if (!(diag_arg->f->families & (1 << r->idiag_family)))
return 0;
- if (diag_arg->f->kill && kill_inet_sock(addr, h, arg) != 0) {
+ if (diag_arg->f->kill && kill_inet_sock(h, arg) != 0) {
if (errno == EOPNOTSUPP || errno == ENOENT) {
/* Socket can't be closed, or is already closed. */
return 0;
--
1.8.2.1
^ permalink raw reply related
* Re: [PATCH v3 net-next 00/11] ipv6: Enable GUEoIPv6 and more fixes for v6 tunneling
From: Alexander Duyck @ 2016-05-09 21:35 UTC (permalink / raw)
To: Tom Herbert; +Cc: David Miller, Netdev, Kernel Team
In-Reply-To: <CAKgT0UeA2feSNSP_LfxNbH-ZdwmjDNjjDSy3uE6qycKVN96Amw@mail.gmail.com>
On Mon, May 9, 2016 at 10:32 AM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Mon, May 9, 2016 at 9:56 AM, Tom Herbert <tom@herbertland.com> wrote:
>> On Fri, May 6, 2016 at 8:03 PM, Alexander Duyck
>> <alexander.duyck@gmail.com> wrote:
>>> On Fri, May 6, 2016 at 7:11 PM, Tom Herbert <tom@herbertland.com> wrote:
>>>> On Fri, May 6, 2016 at 7:03 PM, Alexander Duyck
>>>> <alexander.duyck@gmail.com> wrote:
>>>>> On Fri, May 6, 2016 at 6:57 PM, Tom Herbert <tom@herbertland.com> wrote:
>>>>>> On Fri, May 6, 2016 at 6:09 PM, Alexander Duyck
>>>>>> <alexander.duyck@gmail.com> wrote:
>>>>>>> On Fri, May 6, 2016 at 3:11 PM, Tom Herbert <tom@herbertland.com> wrote:
>>>>>>>> This patch set:
>>>>>>>> - Fixes GRE6 to process translate flags correctly from configuration
>>>>>>>> - Adds support for GSO and GRO for ip6ip6 and ip4ip6
>>>>>>>> - Add support for FOU and GUE in IPv6
>>>>>>>> - Support GRE, ip6ip6 and ip4ip6 over FOU/GUE
>>>>>>>> - Fixes ip6_input to deal with UDP encapsulations
>>>>>>>> - Some other minor fixes
>>>>>>>>
>>>>>>>> v2:
>>>>>>>> - Removed a check of GSO types in MPLS
>>>>>>>> - Define GSO type SKB_GSO_IPXIP6 and SKB_GSO_IPXIP4 (based on input
>>>>>>>> from Alexander)
>>>>>>>> - Don't define GSO types specifally for IP6IP6 and IP4IP6, above
>>>>>>>> fix makes that uncessary
>>>>>>>> - Don't bother clearing encapsulation flag in UDP tunnel segment
>>>>>>>> (another item suggested by Alexander).
>>>>>>>>
>>>>>>>> v3:
>>>>>>>> - Address some minor comments from Alexander
>>>>>>>>
>>>>>>>> Tested:
>>>>>>>> Tested a variety of case, but not the full matrix (which is quite
>>>>>>>> large now). Most of the obivous cases (e.g. GRE) work fine. Still
>>>>>>>> some issues probably with GSO/GRO being effective in all cases.
>>>>>>>>
>>>>>>>> - IPv4/GRE/GUE/IPv6 with RCO
>>>>>>>> 1 TCP_STREAM
>>>>>>>> 6616 Mbps
>>>>>>>> 200 TCP_RR
>>>>>>>> 1244043 tps
>>>>>>>> 141/243/446 90/95/99% latencies
>>>>>>>> 86.61% CPU utilization
>>>>>>>> - IPv6/GRE/GUE/IPv6 with RCO
>>>>>>>> 1 TCP_STREAM
>>>>>>>> 6940 Mbps
>>>>>>>> 200 TCP_RR
>>>>>>>> 1270903 tps
>>>>>>>> 138/236/440 90/95/99% latencies
>>>>>>>> 87.51% CPU utilization
>>>>>>>>
>>>>>>>> - IP6IP6
>>>>>>>> 1 TCP_STREAM
>>>>>>>> 2576 Mbps
>>>>>>>> 200 TCP_RR
>>>>>>>> 498981 tps
>>>>>>>> 388/498/631 90/95/99% latencies
>>>>>>>> 19.75% CPU utilization (1 CPU saturated)
>>>>>>>>
>>>>>>>> - IP6IP6/GUE/IPv6 with RCO
>>>>>>>> 1 TCP_STREAM
>>>>>>>> 1854 Mbps
>>>>>>>> 200 TCP_RR
>>>>>>>> 1233818 tps
>>>>>>>> 143/244/451 90/95/99% latencies
>>>>>>>> 87.57 CPU utilization
>>>>>>>>
>>>>>>>> - IP4IP6
>>>>>>>> 1 TCP_STREAM
>>>>>>>> 200 TCP_RR
>>>>>>>> 763774 tps
>>>>>>>> 250/318/466 90/95/99% latencies
>>>>>>>> 35.25% CPU utilization (1 CPU saturated)
>>>>>>>>
>>>>>>>> - GRE with keyid
>>>>>>>> 200 TCP_RR
>>>>>>>> 744173 tps
>>>>>>>> 258/332/461 90/95/99% latencies
>>>>>>>> 34.59% CPU utilization (1 CPU saturated)
>>>>>>>
>>>>>>> So I tried testing your patch set and it looks like I cannot get GRE
>>>>>>> working for any netperf test. If I pop the patches off it is even
>>>>>>> worse since it looks like patch 3 fixes some tunnel flags issues, but
>>>>>>> still doesn't resolve all the issues introduced with b05229f44228
>>>>>>> ("gre6: Cleanup GREv6 transmit path, call common GRE functions").
>>>>>>> Reverting the entire patch seems to resolve the issues, but I will try
>>>>>>> to pick it apart tonight to see if I can find the other issues that
>>>>>>> weren't addressed in this patch series.
>>>>>>>
>>>>>>
>>>>>> Can you give details about configuration, test you're running, and HW?
>>>>>
>>>>> The issue looks like it may be specific to ip6gretap. I'm running the
>>>>> test over an i40e adapter, but it shouldn't make much difference. I'm
>>>>> thinking it may have something to do with the MTU configuration as
>>>>> that is one of the things I am noticing has changed between the
>>>>> working and the broken version of the code.
>>>>>
>>>> I'm not seeing any issue with configuring:
>>>>
>>>> ip link add name tun8 type ip6gretap remote
>>>> 2401:db00:20:911a:face:0:27:0 local 2401:db00:20:911a:face:0:25:0 ttl
>>>> 225
>>>>
>>>> MTU issues would not surprise me with IPv6 though. This is part of the
>>>> area of code that seems drastically different than what IPv4 is doing.
>>>
>>> I am also using a key.
>>>
>>> ip link add $name type ip6gretap key $net \
>>> local fec0::1 remote $addr6 ttl 225 dev $PF0
>>>
>> I don't see any issue with key enabled.
>>
>>> Does the device you are using support any kind of checksum offload for
>>> inner headers on GRE tunnels? It looks like if I turn off checksums
>>
>> I don't believe so.
>>
>>> and correct the MTU I can then send traffic without issues. I'd say
>>> that the Tx cleanup probably introduced 3 regressions. The first one
>>> you addressed in patch 3 which fixes the flags. The second being the
>>> fact that the MTU is wrong, and the third being something that
>>> apparently broke checksum and maybe segmentation offload for
>>> ip6gretap.
>>>
>> The MTU can be set in place in IPv6 code that doesn't exist in Ipv4. I
>> am especially wondering about the "if (p->flags & IP6_TNL_F_CAP_XMIT)"
>> block.
So I think I have figured out the MTU problem. You need to go through
and audit the spots where you are using GRE_ flags instead of TUNNEL_
flags. Specifically in ip6gre_tnl_link_config you are using GRE_
flags to test o_flags which is configured to use the TUNNEL_ prefixed
flags. When I changed those out then the MTU started coming out
correct again. There were a couple other places you were using
GRE_SEQ where you should have been using TUNNEL_SEQ as well so you
could probably clean those up and add them to patch 3 of your set that
was fixing the flags so that they should be TUNNEL_ prefixed.
>>> Really I think the transmit path cleanup should have probably been
>>> broken down into a set of patches rather than slamming it in all in
>>> one block. I can spend some time next week trying to sort it out if
>>> you don't have any hardware that supports GRE segmentation or checksum
>>> offload. If worse comes to worse I will just try breaking the revert
>>> down into a set of smaller patches so I can figure out exactly which
>>> change broke things.
>>>
>> I am still trying to reproduce.
>
> What NICs are you testing with? Depending on the NIC I might be able
> to point you in the direction of something that can reproduce the
> issue.
>
> At this point I am thinking it is an issue with a header offset since
> I believe GSO resets all that and probably corrects the issue.
I'm still doing some digging on my end. I'm hoping to have this
figured out by the end of today.
- Alex
^ permalink raw reply
* Re: [PATCH v3 net-next 00/11] ipv6: Enable GUEoIPv6 and more fixes for v6 tunneling
From: Tom Herbert @ 2016-05-09 21:37 UTC (permalink / raw)
To: Alexander Duyck; +Cc: David Miller, Netdev, Kernel Team
In-Reply-To: <CAKgT0Ucnr=BzOexzHJ8UXSktAU5Ji9nkmt23+gpQvJxpO3ug=Q@mail.gmail.com>
On Mon, May 9, 2016 at 2:35 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Mon, May 9, 2016 at 10:32 AM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> On Mon, May 9, 2016 at 9:56 AM, Tom Herbert <tom@herbertland.com> wrote:
>>> On Fri, May 6, 2016 at 8:03 PM, Alexander Duyck
>>> <alexander.duyck@gmail.com> wrote:
>>>> On Fri, May 6, 2016 at 7:11 PM, Tom Herbert <tom@herbertland.com> wrote:
>>>>> On Fri, May 6, 2016 at 7:03 PM, Alexander Duyck
>>>>> <alexander.duyck@gmail.com> wrote:
>>>>>> On Fri, May 6, 2016 at 6:57 PM, Tom Herbert <tom@herbertland.com> wrote:
>>>>>>> On Fri, May 6, 2016 at 6:09 PM, Alexander Duyck
>>>>>>> <alexander.duyck@gmail.com> wrote:
>>>>>>>> On Fri, May 6, 2016 at 3:11 PM, Tom Herbert <tom@herbertland.com> wrote:
>>>>>>>>> This patch set:
>>>>>>>>> - Fixes GRE6 to process translate flags correctly from configuration
>>>>>>>>> - Adds support for GSO and GRO for ip6ip6 and ip4ip6
>>>>>>>>> - Add support for FOU and GUE in IPv6
>>>>>>>>> - Support GRE, ip6ip6 and ip4ip6 over FOU/GUE
>>>>>>>>> - Fixes ip6_input to deal with UDP encapsulations
>>>>>>>>> - Some other minor fixes
>>>>>>>>>
>>>>>>>>> v2:
>>>>>>>>> - Removed a check of GSO types in MPLS
>>>>>>>>> - Define GSO type SKB_GSO_IPXIP6 and SKB_GSO_IPXIP4 (based on input
>>>>>>>>> from Alexander)
>>>>>>>>> - Don't define GSO types specifally for IP6IP6 and IP4IP6, above
>>>>>>>>> fix makes that uncessary
>>>>>>>>> - Don't bother clearing encapsulation flag in UDP tunnel segment
>>>>>>>>> (another item suggested by Alexander).
>>>>>>>>>
>>>>>>>>> v3:
>>>>>>>>> - Address some minor comments from Alexander
>>>>>>>>>
>>>>>>>>> Tested:
>>>>>>>>> Tested a variety of case, but not the full matrix (which is quite
>>>>>>>>> large now). Most of the obivous cases (e.g. GRE) work fine. Still
>>>>>>>>> some issues probably with GSO/GRO being effective in all cases.
>>>>>>>>>
>>>>>>>>> - IPv4/GRE/GUE/IPv6 with RCO
>>>>>>>>> 1 TCP_STREAM
>>>>>>>>> 6616 Mbps
>>>>>>>>> 200 TCP_RR
>>>>>>>>> 1244043 tps
>>>>>>>>> 141/243/446 90/95/99% latencies
>>>>>>>>> 86.61% CPU utilization
>>>>>>>>> - IPv6/GRE/GUE/IPv6 with RCO
>>>>>>>>> 1 TCP_STREAM
>>>>>>>>> 6940 Mbps
>>>>>>>>> 200 TCP_RR
>>>>>>>>> 1270903 tps
>>>>>>>>> 138/236/440 90/95/99% latencies
>>>>>>>>> 87.51% CPU utilization
>>>>>>>>>
>>>>>>>>> - IP6IP6
>>>>>>>>> 1 TCP_STREAM
>>>>>>>>> 2576 Mbps
>>>>>>>>> 200 TCP_RR
>>>>>>>>> 498981 tps
>>>>>>>>> 388/498/631 90/95/99% latencies
>>>>>>>>> 19.75% CPU utilization (1 CPU saturated)
>>>>>>>>>
>>>>>>>>> - IP6IP6/GUE/IPv6 with RCO
>>>>>>>>> 1 TCP_STREAM
>>>>>>>>> 1854 Mbps
>>>>>>>>> 200 TCP_RR
>>>>>>>>> 1233818 tps
>>>>>>>>> 143/244/451 90/95/99% latencies
>>>>>>>>> 87.57 CPU utilization
>>>>>>>>>
>>>>>>>>> - IP4IP6
>>>>>>>>> 1 TCP_STREAM
>>>>>>>>> 200 TCP_RR
>>>>>>>>> 763774 tps
>>>>>>>>> 250/318/466 90/95/99% latencies
>>>>>>>>> 35.25% CPU utilization (1 CPU saturated)
>>>>>>>>>
>>>>>>>>> - GRE with keyid
>>>>>>>>> 200 TCP_RR
>>>>>>>>> 744173 tps
>>>>>>>>> 258/332/461 90/95/99% latencies
>>>>>>>>> 34.59% CPU utilization (1 CPU saturated)
>>>>>>>>
>>>>>>>> So I tried testing your patch set and it looks like I cannot get GRE
>>>>>>>> working for any netperf test. If I pop the patches off it is even
>>>>>>>> worse since it looks like patch 3 fixes some tunnel flags issues, but
>>>>>>>> still doesn't resolve all the issues introduced with b05229f44228
>>>>>>>> ("gre6: Cleanup GREv6 transmit path, call common GRE functions").
>>>>>>>> Reverting the entire patch seems to resolve the issues, but I will try
>>>>>>>> to pick it apart tonight to see if I can find the other issues that
>>>>>>>> weren't addressed in this patch series.
>>>>>>>>
>>>>>>>
>>>>>>> Can you give details about configuration, test you're running, and HW?
>>>>>>
>>>>>> The issue looks like it may be specific to ip6gretap. I'm running the
>>>>>> test over an i40e adapter, but it shouldn't make much difference. I'm
>>>>>> thinking it may have something to do with the MTU configuration as
>>>>>> that is one of the things I am noticing has changed between the
>>>>>> working and the broken version of the code.
>>>>>>
>>>>> I'm not seeing any issue with configuring:
>>>>>
>>>>> ip link add name tun8 type ip6gretap remote
>>>>> 2401:db00:20:911a:face:0:27:0 local 2401:db00:20:911a:face:0:25:0 ttl
>>>>> 225
>>>>>
>>>>> MTU issues would not surprise me with IPv6 though. This is part of the
>>>>> area of code that seems drastically different than what IPv4 is doing.
>>>>
>>>> I am also using a key.
>>>>
>>>> ip link add $name type ip6gretap key $net \
>>>> local fec0::1 remote $addr6 ttl 225 dev $PF0
>>>>
>>> I don't see any issue with key enabled.
>>>
>>>> Does the device you are using support any kind of checksum offload for
>>>> inner headers on GRE tunnels? It looks like if I turn off checksums
>>>
>>> I don't believe so.
>>>
>>>> and correct the MTU I can then send traffic without issues. I'd say
>>>> that the Tx cleanup probably introduced 3 regressions. The first one
>>>> you addressed in patch 3 which fixes the flags. The second being the
>>>> fact that the MTU is wrong, and the third being something that
>>>> apparently broke checksum and maybe segmentation offload for
>>>> ip6gretap.
>>>>
>>> The MTU can be set in place in IPv6 code that doesn't exist in Ipv4. I
>>> am especially wondering about the "if (p->flags & IP6_TNL_F_CAP_XMIT)"
>>> block.
>
> So I think I have figured out the MTU problem. You need to go through
> and audit the spots where you are using GRE_ flags instead of TUNNEL_
> flags. Specifically in ip6gre_tnl_link_config you are using GRE_
> flags to test o_flags which is configured to use the TUNNEL_ prefixed
> flags. When I changed those out then the MTU started coming out
> correct again. There were a couple other places you were using
> GRE_SEQ where you should have been using TUNNEL_SEQ as well so you
> could probably clean those up and add them to patch 3 of your set that
> was fixing the flags so that they should be TUNNEL_ prefixed.
>
>>>> Really I think the transmit path cleanup should have probably been
>>>> broken down into a set of patches rather than slamming it in all in
>>>> one block. I can spend some time next week trying to sort it out if
>>>> you don't have any hardware that supports GRE segmentation or checksum
>>>> offload. If worse comes to worse I will just try breaking the revert
>>>> down into a set of smaller patches so I can figure out exactly which
>>>> change broke things.
>>>>
>>> I am still trying to reproduce.
>>
>> What NICs are you testing with? Depending on the NIC I might be able
>> to point you in the direction of something that can reproduce the
>> issue.
>>
>> At this point I am thinking it is an issue with a header offset since
>> I believe GSO resets all that and probably corrects the issue.
>
> I'm still doing some digging on my end. I'm hoping to have this
> figured out by the end of today.
>
Yes, I will be sending a patch shortly for that if you can try it.
Tom
> - Alex
^ 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