* Re: [PATCH v2 03/10] cgroup: net_cls: Protect access to task_cls_classid() when built as module
From: Paul E. McKenney @ 2012-08-28 14:47 UTC (permalink / raw)
To: Daniel Wagner
Cc: Tejun Heo, netdev-u79uwXL29TY76Z2rM5mHXA,
cgroups-u79uwXL29TY76Z2rM5mHXA, David S. Miller, Gao feng,
Jamal Hadi Salim, John Fastabend, Li Zefan, Neil Horman
In-Reply-To: <503903BD.6020208-kQCPcA+X3s7YtjvyW6yDsg@public.gmane.org>
On Sat, Aug 25, 2012 at 06:56:29PM +0200, Daniel Wagner wrote:
> On 25.08.2012 01:26, Tejun Heo wrote:
> >On Fri, Aug 24, 2012 at 04:01:37PM +0200, Daniel Wagner wrote:
> >>@@ -306,6 +312,11 @@ static void __exit exit_cgroup_cls(void)
> >> synchronize_rcu();
> >> #endif
> >>
> >>+#if IS_MODULE(CONFIG_NET_CLS_CGROUP)
> >>+ static_key_slow_dec(&cgroup_cls_enabled);
> >>+ rcu_barrier();
> >
> >Why is this rcu_barrier() necessary?
>
> I have read the rcubarrier.txt document and I got from that that an
> rcu_barrier() is needed when unloading a module. But maybe I got it
> wrong.
>
> So the idea after disabling the jump lables all pending readers in
> task_cls_classid() have left. THe same thing is done in the old code
> with the dynamic id part. With the difference that synchronize_rcu()
> is used.
FWIW, the rcu_barrier() is needed only if the module uses call_rcu().
In that case it is required to ensure that all the resulting callbacks
execute before the module's .text is freed up.
Thanx, Paul
> >In general, please explain what
> >synchronization is going on when using sync constructs which aren't
> >obvious - e.g. memory barriers, rcu barriers.
>
> Sure, I will keep this in mind.
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH] rtlwifi: rtl8192de: remove pointless conditional before kfree_skb()
From: Larry Finger @ 2012-08-28 14:44 UTC (permalink / raw)
To: Wei Yongjun
Cc: chaoming_li-kXabqFNEczNtrwSWzY7KCg,
linville-2XuSBdqkA4R54TAoqtyWWQ,
yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CAPgLHd86W5X=Tb_fXoz_Ood-w_s282QNePTXG9m_6ESdG-rG2A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 08/28/2012 08:11 AM, Wei Yongjun wrote:
> From: Wei Yongjun <yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY@public.gmane.org>
>
> Remove pointless conditional before kfree_skb().
>
> Signed-off-by: Wei Yongjun <yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY@public.gmane.org>
> ---
> drivers/net/wireless/rtlwifi/rtl8192de/fw.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
Acked-by: Larry Finger <Larry.Finger.net>
Thanks,
Larry
>
> diff --git a/drivers/net/wireless/rtlwifi/rtl8192de/fw.c b/drivers/net/wireless/rtlwifi/rtl8192de/fw.c
> index 895ae6c..a3aede0 100644
> --- a/drivers/net/wireless/rtlwifi/rtl8192de/fw.c
> +++ b/drivers/net/wireless/rtlwifi/rtl8192de/fw.c
> @@ -570,8 +570,7 @@ static bool _rtl92d_cmd_send_packet(struct ieee80211_hw *hw,
>
> ring = &rtlpci->tx_ring[BEACON_QUEUE];
> pskb = __skb_dequeue(&ring->queue);
> - if (pskb)
> - kfree_skb(pskb);
> + kfree_skb(pskb);
> spin_lock_irqsave(&rtlpriv->locks.irq_th_lock, flags);
> pdesc = &ring->desc[idx];
> /* discard output from call below */
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] rtlwifi: rtl8192c: remove pointless conditional before kfree_skb()
From: Larry Finger @ 2012-08-28 14:44 UTC (permalink / raw)
To: Wei Yongjun; +Cc: chaoming_li, linville, yongjun_wei, linux-wireless, netdev
In-Reply-To: <CAPgLHd_q0T=RD+qgEzFpnhez4Hdu-jDuFf0E+MgwiWLSeaMYaQ@mail.gmail.com>
On 08/28/2012 08:12 AM, Wei Yongjun wrote:
> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
>
> Remove pointless conditional before kfree_skb().
>
> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> ---
> drivers/net/wireless/rtlwifi/rtl8192c/fw_common.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
Acked-by: Larry Finger <Larry.Finger.net>
Thanks,
Larry
>
> diff --git a/drivers/net/wireless/rtlwifi/rtl8192c/fw_common.c b/drivers/net/wireless/rtlwifi/rtl8192c/fw_common.c
> index 44febfd..b627151 100644
> --- a/drivers/net/wireless/rtlwifi/rtl8192c/fw_common.c
> +++ b/drivers/net/wireless/rtlwifi/rtl8192c/fw_common.c
> @@ -577,8 +577,7 @@ static bool _rtl92c_cmd_send_packet(struct ieee80211_hw *hw,
> ring = &rtlpci->tx_ring[BEACON_QUEUE];
>
> pskb = __skb_dequeue(&ring->queue);
> - if (pskb)
> - kfree_skb(pskb);
> + kfree_skb(pskb);
>
> spin_lock_irqsave(&rtlpriv->locks.irq_th_lock, flags);
>
>
>
>
^ permalink raw reply
* [PATCH V2 2/2] ipvs: Extend MTU check to account for IPv6 NAT defrag changes
From: Jesper Dangaard Brouer @ 2012-08-28 14:23 UTC (permalink / raw)
To: netdev, Patrick McHardy, lvs-devel, Julian Anastasov,
Simon Horman, Pablo Neira Ayuso
Cc: Jesper Dangaard Brouer, Hans Schillstrom, Wensong Zhang,
netfilter-devel
In-Reply-To: <20120828142051.11475.63775.stgit@dragon>
This patch is necessary, to make IPVS work, after Patrick McHardys
IPv6 NAT defragmentation changes.
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
In V2: the tunnel mode is no longer a special case.
net/netfilter/ipvs/ip_vs_xmit.c | 9 ++++++++-
1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
index 67a3978..56f6d5d 100644
--- a/net/netfilter/ipvs/ip_vs_xmit.c
+++ b/net/netfilter/ipvs/ip_vs_xmit.c
@@ -88,7 +88,14 @@ __ip_vs_dst_check(struct ip_vs_dest *dest, u32 rtos)
static inline bool
__mtu_check_toobig_v6(const struct sk_buff *skb, u32 mtu)
{
- if (skb->len > mtu && !skb_is_gso(skb)) {
+ if (IP6CB(skb)->frag_max_size) {
+ /* frag_max_size tell us that, this packet have been
+ * defragmented by netfilter IPv6 conntrack module.
+ */
+ if (IP6CB(skb)->frag_max_size > mtu)
+ return true; /* largest fragment violate MTU */
+ }
+ else if (skb->len > mtu && !skb_is_gso(skb)) {
return true; /* Packet size violate MTU size */
}
return false;
^ permalink raw reply related
* [PATCH V2 1/2] ipvs: IPv6 MTU checking cleanup and bugfix
From: Jesper Dangaard Brouer @ 2012-08-28 14:22 UTC (permalink / raw)
To: netdev, Patrick McHardy, lvs-devel, Julian Anastasov,
Simon Horman, Pablo Neira Ayuso
Cc: Jesper Dangaard Brouer, Hans Schillstrom, Wensong Zhang,
netfilter-devel
In-Reply-To: <20120828142051.11475.63775.stgit@dragon>
Cleaning up the IPv6 MTU checking in the IPVS xmit code, by using
a common helper function __mtu_check_toobig_v6().
The MTU check for tunnel mode can also use this helper as
ntohs(old_iph->payload_len) + sizeof(struct ipv6hdr) is qual to
skb->len. And the 'mtu' variable have been adjusted before
calling helper.
Notice, this also fixes a bug, as the the MTU check in ip_vs_dr_xmit_v6()
were missing a check for skb_is_gso().
This bug e.g. caused issues for KVM IPVS setups, where different
Segmentation Offloading techniques are utilized, between guests,
via the virtio driver. This resulted in very bad performance,
due to the ICMPv6 "too big" messages didn't affect the sender.
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
In V2: we also make tunnel mode use the common helper
net/netfilter/ipvs/ip_vs_xmit.c | 21 +++++++++++++++------
1 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
index 543a554..67a3978 100644
--- a/net/netfilter/ipvs/ip_vs_xmit.c
+++ b/net/netfilter/ipvs/ip_vs_xmit.c
@@ -85,6 +85,15 @@ __ip_vs_dst_check(struct ip_vs_dest *dest, u32 rtos)
return dst;
}
+static inline bool
+__mtu_check_toobig_v6(const struct sk_buff *skb, u32 mtu)
+{
+ if (skb->len > mtu && !skb_is_gso(skb)) {
+ return true; /* Packet size violate MTU size */
+ }
+ return false;
+}
+
/* Get route to daddr, update *saddr, optionally bind route to saddr */
static struct rtable *do_output_route4(struct net *net, __be32 daddr,
u32 rtos, int rt_mode, __be32 *saddr)
@@ -491,7 +500,7 @@ ip_vs_bypass_xmit_v6(struct sk_buff *skb, struct ip_vs_conn *cp,
/* MTU checking */
mtu = dst_mtu(&rt->dst);
- if (skb->len > mtu && !skb_is_gso(skb)) {
+ if (__mtu_check_toobig_v6(skb, mtu)) {
if (!skb->dev) {
struct net *net = dev_net(skb_dst(skb)->dev);
@@ -712,7 +721,7 @@ ip_vs_nat_xmit_v6(struct sk_buff *skb, struct ip_vs_conn *cp,
/* MTU checking */
mtu = dst_mtu(&rt->dst);
- if (skb->len > mtu && !skb_is_gso(skb)) {
+ if (__mtu_check_toobig_v6(skb, mtu)) {
if (!skb->dev) {
struct net *net = dev_net(skb_dst(skb)->dev);
@@ -946,8 +955,8 @@ ip_vs_tunnel_xmit_v6(struct sk_buff *skb, struct ip_vs_conn *cp,
if (skb_dst(skb))
skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
- if (mtu < ntohs(old_iph->payload_len) + sizeof(struct ipv6hdr) &&
- !skb_is_gso(skb)) {
+ /* MTU checking: Notice that 'mtu' have been adjusted before hand */
+ if (__mtu_check_toobig_v6(skb, mtu)) {
if (!skb->dev) {
struct net *net = dev_net(skb_dst(skb)->dev);
@@ -1113,7 +1122,7 @@ ip_vs_dr_xmit_v6(struct sk_buff *skb, struct ip_vs_conn *cp,
/* MTU checking */
mtu = dst_mtu(&rt->dst);
- if (skb->len > mtu) {
+ if (__mtu_check_toobig_v6(skb, mtu)) {
if (!skb->dev) {
struct net *net = dev_net(skb_dst(skb)->dev);
@@ -1349,7 +1358,7 @@ ip_vs_icmp_xmit_v6(struct sk_buff *skb, struct ip_vs_conn *cp,
/* MTU checking */
mtu = dst_mtu(&rt->dst);
- if (skb->len > mtu && !skb_is_gso(skb)) {
+ if (__mtu_check_toobig_v6(skb, mtu)) {
if (!skb->dev) {
struct net *net = dev_net(skb_dst(skb)->dev);
^ permalink raw reply related
* [PATCH V2 0/2] net: ipvs and netfilter IPv6 defrag MTU handling
From: Jesper Dangaard Brouer @ 2012-08-28 14:21 UTC (permalink / raw)
To: netdev, Patrick McHardy, lvs-devel, Julian Anastasov,
Simon Horman, Pablo Neira Ayuso
Cc: Jesper Dangaard Brouer, Hans Schillstrom, Wensong Zhang,
netfilter-devel
In-Reply-To: <Pine.GSO.4.63.1208281020530.18770@stinky-local.trash.net>
The following patchset V2 makes IPVS compatible with Patrick McHardys
IPv6 NAT patchset: http://thread.gmane.org/gmane.linux.network/239615
Specifically the part that improves defragmentation.
Patch01: Cleanup (and fixes a bug) in IPVS MTU IPv6 handling
In V2: we also handle tunnel mode, to use the common helper
Patch02: Extend MTU check to account for IPv6 NAT defrag changes
In V2: the tunnel mode is no longer a special case.
This patchset is based upon:
Homes ipvs-next tree:
git://git.kernel.org/pub/scm/linux/kernel/git/horms/ipvs-next.git
BUT with Patrick McHardy's IPv6 NAT patchset applied first, on top of
commit: 3654e61137db891f5312e6dd813b961484b5fdf3
title: "ipvs: add pmtu_disc option to disable IP DF for TUN packets"
---
Jesper Dangaard Brouer (2):
ipvs: Extend MTU check to account for IPv6 NAT defrag changes
ipvs: IPv6 MTU checking cleanup and bugfix
net/netfilter/ipvs/ip_vs_xmit.c | 28 ++++++++++++++++++++++------
1 files changed, 22 insertions(+), 6 deletions(-)
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Sr. Network Kernel Developer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH] cxgb3: remove pointless conditional before kfree_skb()
From: Wei Yongjun @ 2012-08-28 14:19 UTC (permalink / raw)
To: David.Laight; +Cc: divy, yongjun_wei, netdev
On 08/28/2012 09:24 PM, David Laight wrote:
>> - if (t->nofail_skb)
>> - kfree_skb(t->nofail_skb);
>> + kfree_skb(t->nofail_skb);
> Some of these conditionals might be there for performance
> reasons - eg if common path and normally NULL.
I have a quick look at those code, cxgb3_offload_deactivate()
is only used when remove device or device error and then disable
the device, so maybe it is not a common path.
^ permalink raw reply
* Re: [PATCH] skbuff: remove pointless conditional before kfree_skb()
From: Eric Dumazet @ 2012-08-28 14:12 UTC (permalink / raw)
To: Wei Yongjun; +Cc: davem, yongjun_wei, netdev
In-Reply-To: <CAPgLHd9m2VbFFXt5Q3MMrK6BPXvMrT-ptq+RfgBToXOpxAKOOw@mail.gmail.com>
On Tue, 2012-08-28 at 21:10 +0800, Wei Yongjun wrote:
> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
>
> Remove pointless conditional before kfree_skb().
>
> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> ---
> include/linux/skbuff.h | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 7632c87..0b846d9 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -2464,8 +2464,7 @@ static inline void nf_conntrack_get_reasm(struct sk_buff *skb)
> }
> static inline void nf_conntrack_put_reasm(struct sk_buff *skb)
> {
> - if (skb)
> - kfree_skb(skb);
> + kfree_skb(skb);
> }
> #endif
> #ifdef CONFIG_BRIDGE_NETFILTER
>
Its not exactly pointless.
Its a tradeoff between kernel code size, and ability for cpu to predict
a branch in kfree_skb()
This test is in hot path, and therefore this patch can potentially have
a performance impact.
I really think most kfree_skb() calls are done with a non NULL param,
so the branch prediction is good.
But after this patch, things are totally different.
Therefore, I am against it.
^ permalink raw reply
* RE: [PATCH] cxgb3: remove pointless conditional before kfree_skb()
From: David Laight @ 2012-08-28 13:24 UTC (permalink / raw)
To: Wei Yongjun, divy; +Cc: yongjun_wei, netdev
In-Reply-To: <CAPgLHd9b+0DDZ9iYFzNS4NP1w_3GmWaMD+ihqeVeEGXei9cfUg@mail.gmail.com>
> - if (t->nofail_skb)
> - kfree_skb(t->nofail_skb);
> + kfree_skb(t->nofail_skb);
Some of these conditionals might be there for performance
reasons - eg if common path and normally NULL.
David
^ permalink raw reply
* [PATCH] mISDN: remove pointless conditional before kfree_skb()
From: Wei Yongjun @ 2012-08-28 13:12 UTC (permalink / raw)
To: isdn; +Cc: yongjun_wei, netdev
From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
Remove pointless conditional before kfree_skb().
Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
---
drivers/isdn/mISDN/socket.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/isdn/mISDN/socket.c b/drivers/isdn/mISDN/socket.c
index abe2d69..f677724 100644
--- a/drivers/isdn/mISDN/socket.c
+++ b/drivers/isdn/mISDN/socket.c
@@ -243,8 +243,7 @@ mISDN_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
}
done:
- if (skb)
- kfree_skb(skb);
+ kfree_skb(skb);
release_sock(sk);
return err;
}
^ permalink raw reply related
* [PATCH] rtlwifi: rtl8192c: remove pointless conditional before kfree_skb()
From: Wei Yongjun @ 2012-08-28 13:12 UTC (permalink / raw)
To: Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ,
chaoming_li-kXabqFNEczNtrwSWzY7KCg,
linville-2XuSBdqkA4R54TAoqtyWWQ
Cc: yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
From: Wei Yongjun <yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY@public.gmane.org>
Remove pointless conditional before kfree_skb().
Signed-off-by: Wei Yongjun <yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY@public.gmane.org>
---
drivers/net/wireless/rtlwifi/rtl8192c/fw_common.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/wireless/rtlwifi/rtl8192c/fw_common.c b/drivers/net/wireless/rtlwifi/rtl8192c/fw_common.c
index 44febfd..b627151 100644
--- a/drivers/net/wireless/rtlwifi/rtl8192c/fw_common.c
+++ b/drivers/net/wireless/rtlwifi/rtl8192c/fw_common.c
@@ -577,8 +577,7 @@ static bool _rtl92c_cmd_send_packet(struct ieee80211_hw *hw,
ring = &rtlpci->tx_ring[BEACON_QUEUE];
pskb = __skb_dequeue(&ring->queue);
- if (pskb)
- kfree_skb(pskb);
+ kfree_skb(pskb);
spin_lock_irqsave(&rtlpriv->locks.irq_th_lock, flags);
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH] rtlwifi: rtl8192de: remove pointless conditional before kfree_skb()
From: Wei Yongjun @ 2012-08-28 13:11 UTC (permalink / raw)
To: Larry.Finger, chaoming_li, linville; +Cc: yongjun_wei, linux-wireless, netdev
From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
Remove pointless conditional before kfree_skb().
Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
---
drivers/net/wireless/rtlwifi/rtl8192de/fw.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/wireless/rtlwifi/rtl8192de/fw.c b/drivers/net/wireless/rtlwifi/rtl8192de/fw.c
index 895ae6c..a3aede0 100644
--- a/drivers/net/wireless/rtlwifi/rtl8192de/fw.c
+++ b/drivers/net/wireless/rtlwifi/rtl8192de/fw.c
@@ -570,8 +570,7 @@ static bool _rtl92d_cmd_send_packet(struct ieee80211_hw *hw,
ring = &rtlpci->tx_ring[BEACON_QUEUE];
pskb = __skb_dequeue(&ring->queue);
- if (pskb)
- kfree_skb(pskb);
+ kfree_skb(pskb);
spin_lock_irqsave(&rtlpriv->locks.irq_th_lock, flags);
pdesc = &ring->desc[idx];
/* discard output from call below */
^ permalink raw reply related
* [PATCH] skbuff: remove pointless conditional before kfree_skb()
From: Wei Yongjun @ 2012-08-28 13:10 UTC (permalink / raw)
To: davem; +Cc: yongjun_wei, netdev
From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
Remove pointless conditional before kfree_skb().
Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
---
include/linux/skbuff.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 7632c87..0b846d9 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2464,8 +2464,7 @@ static inline void nf_conntrack_get_reasm(struct sk_buff *skb)
}
static inline void nf_conntrack_put_reasm(struct sk_buff *skb)
{
- if (skb)
- kfree_skb(skb);
+ kfree_skb(skb);
}
#endif
#ifdef CONFIG_BRIDGE_NETFILTER
^ permalink raw reply related
* [PATCH] batman-adv: remove pointless conditional before kfree_skb()
From: Wei Yongjun @ 2012-08-28 13:06 UTC (permalink / raw)
To: lindner_marek, siwu, ordex, davem; +Cc: yongjun_wei, b.a.t.m.a.n, netdev
From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
Remove pointless conditional before kfree_skb().
Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
---
net/batman-adv/send.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/batman-adv/send.c b/net/batman-adv/send.c
index 3b4b2da..534dc46 100644
--- a/net/batman-adv/send.c
+++ b/net/batman-adv/send.c
@@ -99,8 +99,7 @@ void batadv_schedule_bat_ogm(struct batadv_hard_iface *hard_iface)
static void batadv_forw_packet_free(struct batadv_forw_packet *forw_packet)
{
- if (forw_packet->skb)
- kfree_skb(forw_packet->skb);
+ kfree_skb(forw_packet->skb);
if (forw_packet->if_incoming)
batadv_hardif_free_ref(forw_packet->if_incoming);
kfree(forw_packet);
^ permalink raw reply related
* [PATCH] tipc: remove pointless conditional before kfree_skb()
From: Wei Yongjun @ 2012-08-28 13:05 UTC (permalink / raw)
To: jon.maloy, allan.stephens, davem; +Cc: yongjun_wei, netdev, tipc-discussion
From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
Remove pointless conditional before kfree_skb().
Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
---
net/tipc/port.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/net/tipc/port.c b/net/tipc/port.c
index 07c42fb..9227d2c 100644
--- a/net/tipc/port.c
+++ b/net/tipc/port.c
@@ -770,8 +770,7 @@ static void port_dispatcher_sigh(void *dummy)
break;
}
}
- if (buf)
- kfree_skb(buf);
+ kfree_skb(buf);
buf = next;
continue;
err:
@@ -819,8 +818,7 @@ err:
break;
}
}
- if (buf)
- kfree_skb(buf);
+ kfree_skb(buf);
buf = next;
continue;
reject:
^ permalink raw reply related
* [PATCH] cxgb3: remove pointless conditional before kfree_skb()
From: Wei Yongjun @ 2012-08-28 13:04 UTC (permalink / raw)
To: divy; +Cc: yongjun_wei, netdev
From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
Remove pointless conditional before kfree_skb().
Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
---
drivers/net/ethernet/chelsio/cxgb3/cxgb3_offload.c | 3 +--
drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c | 3 +--
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/chelsio/cxgb3/cxgb3_offload.c b/drivers/net/ethernet/chelsio/cxgb3/cxgb3_offload.c
index 2dbbcbb..aac26f2 100644
--- a/drivers/net/ethernet/chelsio/cxgb3/cxgb3_offload.c
+++ b/drivers/net/ethernet/chelsio/cxgb3/cxgb3_offload.c
@@ -1341,8 +1341,7 @@ void cxgb3_offload_deactivate(struct adapter *adapter)
rcu_read_unlock();
RCU_INIT_POINTER(tdev->l2opt, NULL);
call_rcu(&d->rcu_head, clean_l2_data);
- if (t->nofail_skb)
- kfree_skb(t->nofail_skb);
+ kfree_skb(t->nofail_skb);
kfree(t);
}
diff --git a/drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c b/drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c
index 6505070..0b79309 100644
--- a/drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c
@@ -3412,8 +3412,7 @@ static void __devexit remove_one(struct pci_dev *pdev)
free_netdev(adapter->port[i]);
iounmap(adapter->regs);
- if (adapter->nofail_skb)
- kfree_skb(adapter->nofail_skb);
+ kfree_skb(adapter->nofail_skb);
kfree(adapter);
pci_release_regions(pdev);
pci_disable_device(pdev);
^ permalink raw reply related
* [PATCH] NFC: remove pointless conditional before kfree_skb()
From: Wei Yongjun @ 2012-08-28 13:02 UTC (permalink / raw)
To: lauro.venancio, aloisio.almeida, sameo, davem
Cc: yongjun_wei, linux-wireless, netdev
From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
Remove pointless conditional before kfree_skb().
Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
---
net/nfc/hci/shdlc.c | 3 +--
net/nfc/hci/core.c | 3 +--
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/net/nfc/hci/shdlc.c b/net/nfc/hci/shdlc.c
index 6f840c1..52e5cbb 100644
--- a/net/nfc/hci/shdlc.c
+++ b/net/nfc/hci/shdlc.c
@@ -241,8 +241,7 @@ static void nfc_shdlc_rcv_i_frame(struct nfc_shdlc *shdlc,
}
exit:
- if (skb)
- kfree_skb(skb);
+ kfree_skb(skb);
}
static void nfc_shdlc_rcv_ack(struct nfc_shdlc *shdlc, int y_nr)
diff --git a/net/nfc/hci/core.c b/net/nfc/hci/core.c
index 1ac7b3f..35413cc 100644
--- a/net/nfc/hci/core.c
+++ b/net/nfc/hci/core.c
@@ -398,8 +398,7 @@ disconnect_all:
nfc_hci_disconnect_all_gates(hdev);
exit:
- if (skb)
- kfree_skb(skb);
+ kfree_skb(skb);
return r;
}
^ permalink raw reply related
* [PATCH] net: qmi_wwan: new device: Foxconn/Novatel E396
From: Aleksander Morgado @ 2012-08-28 12:30 UTC (permalink / raw)
To: netdev
Cc: linux-usb, Aleksander Morgado, Dan Williams, Bjørn Mork,
Ben Chan
Foxconn-branded Novatel E396, Gobi3k modem.
Cc: Dan Williams <dcbw@redhat.com>
Cc: Bjørn Mork <bjorn@mork.no>
Cc: Ben Chan <benchan@google.com>
Signed-off-by: Aleksander Morgado <aleksander@lanedo.com>
---
drivers/net/usb/qmi_wwan.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index 328397c..189e52d 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -441,6 +441,7 @@ static const struct usb_device_id products[] = {
{QMI_GOBI_DEVICE(0x1199, 0x9015)}, /* Sierra Wireless Gobi 3000 Modem device */
{QMI_GOBI_DEVICE(0x1199, 0x9019)}, /* Sierra Wireless Gobi 3000 Modem device */
{QMI_GOBI_DEVICE(0x1199, 0x901b)}, /* Sierra Wireless MC7770 */
+ {QMI_GOBI_DEVICE(0x1410, 0xa021)}, /* Foxconn Gobi 3000 Modem device (Novatel E396) */
{ } /* END */
};
--
1.7.11.4
^ permalink raw reply related
* Re: [PATCH v3 01/17] hashtable: introduce a small and naive hashtable
From: Mathieu Desnoyers @ 2012-08-28 11:56 UTC (permalink / raw)
To: Sasha Levin
Cc: snitzer-H+wXaHxf7aLQT0dZR+AlfA, neilb-l3A5Bk7waGM,
fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, agk-H+wXaHxf7aLQT0dZR+AlfA,
aarcange-H+wXaHxf7aLQT0dZR+AlfA, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ccaulfie-H+wXaHxf7aLQT0dZR+AlfA, mingo-X9Un+BFzKDI,
dev-yBygre7rU0TnMu66kgdUjQ, ericvh-Re5JQEeQqe8AvxtiuMwx3w,
josh-iaAMLnmF4UmaiuxdJuQwMA, rostedt-nx8X9YLhiw1AfugRpC6u6w,
lw-BthXqXjhjHXQFUHtdCDX3A, teigland-H+wXaHxf7aLQT0dZR+AlfA,
axboe-tSWWG44O7X1aa/9Udqfwiw, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ejt-H+wXaHxf7aLQT0dZR+AlfA,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, Tejun Heo,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <503CAB1E.5010408-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
* Sasha Levin (levinsasha928-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org) wrote:
> On 08/28/2012 12:11 PM, Mathieu Desnoyers wrote:
> > * Sasha Levin (levinsasha928-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org) wrote:
> >> On 08/25/2012 06:24 AM, Mathieu Desnoyers wrote:
> >>> * Tejun Heo (tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org) wrote:
> >>>> Hello,
> >>>>
> >>>> On Sat, Aug 25, 2012 at 12:59:25AM +0200, Sasha Levin wrote:
> >>>>> Thats the thing, the amount of things of things you can do with a given bucket
> >>>>> is very limited. You can't add entries to any point besides the head (without
> >>>>> walking the entire list).
> >>>>
> >>>> Kinda my point. We already have all the hlist*() interface to deal
> >>>> with such cases. Having something which is evidently the trivial
> >>>> hlist hashtable and advertises as such in the interface can be
> >>>> helpful. I think we need that more than we need anything fancy.
> >>>>
> >>>> Heh, this is a debate about which one is less insignificant. I can
> >>>> see your point. I'd really like to hear what others think on this.
> >>>>
> >>>> Guys, do we want something which is evidently trivial hlist hashtable
> >>>> which can use hlist_*() API directly or do we want something better
> >>>> encapsulated?
> >>>
> >>> My 2 cents, FWIW: I think this specific effort should target a trivially
> >>> understandable API and implementation, for use-cases where one would be
> >>> tempted to reimplement his own trivial hash table anyway. So here
> >>> exposing hlist internals, with which kernel developers are already
> >>> familiar, seems like a good approach in my opinion, because hiding stuff
> >>> behind new abstraction might make the target users go away.
> >>>
> >>> Then, as we see the need, we can eventually merge a more elaborate hash
> >>> table with poneys and whatnot, but I would expect that the trivial hash
> >>> table implementation would still be useful. There are of course very
> >>> compelling reasons to use a more featureful hash table: automatic
> >>> resize, RT-aware updates, scalable updates, etc... but I see a purpose
> >>> for a trivial implementation. Its primary strong points being:
> >>>
> >>> - it's trivially understandable, so anyone how want to be really sure
> >>> they won't end up debugging the hash table instead of their
> >>> work-in-progress code can have a full understanding of it,
> >>> - it has few dependencies, which makes it easier to understand and
> >>> easier to use in some contexts (e.g. early boot).
> >>>
> >>> So I'm in favor of not overdoing the abstraction for this trivial hash
> >>> table, and honestly I would rather prefer that this trivial hash table
> >>> stays trivial. A more elaborate hash table should probably come as a
> >>> separate API.
> >>>
> >>> Thanks,
> >>>
> >>> Mathieu
> >>>
> >>
> >> Alright, let's keep it simple then.
> >>
> >> I do want to keep the hash_for_each[rcu,safe] family though.
> >
> > Just a thought: if the API offered by the simple hash table focus on
> > providing a mechanism to find the hash bucket to which belongs the hash
> > chain containing the key looked up, and then expects the user to use the
> > hlist API to iterate on the chain (with or without the hlist _rcu
> > variant), then it might seem consistent that a helper providing
> > iteration over the entire table would actually just provide iteration on
> > all buckets, and let the user call the hlist for each iterator for each
> > node within the bucket, e.g.:
> >
> > struct hlist_head *head;
> > struct hlist_node *pos;
> >
> > hash_for_each_bucket(ht, head) {
> > hlist_for_each(pos, head) {
> > ...
> > }
> > }
> >
> > That way you only have to provide one single macro
> > (hash_for_each_bucket), and rely on the already existing:
> >
> > - hlist_for_each_entry
> > - hlist_for_each_safe
> > - hlist_for_each_entry_rcu
> > - hlist_for_each_safe_rcu
> > .....
> >
> > and various flavors that can appear in the future without duplicating
> > this API. So you won't even have to create _rcu, _safe, nor _safe_rcu
> > versions of the hash_for_each_bucket macro.
> >
> > Thoughts ?
>
> In my opinion, the downside here is that it'll require 2 function calls and 2
> levels of nesting for a simple hash iteration.
Those are macros, not functions. No function call is required. But I see
your point about nesting.
>
> hash_for_each_bucket() will always be followed by an iteration of that
> bucket, so splitting a hash_for_each() which does both into 2
> different functions which will almost always must be called in that
> given order sounds unintuitive to me.
>
> It's also just 3 different possible iterators:
>
> - hlist_for_each_entry
> - hlist_for_each_entry_safe
> - hlist_for_each_entry_rcu
>
> So I think that it's a good price to pay - 2 extra macro definitions
> in the header to save a macro call + nesting level in each place that
> uses a hashtable.
I must admin I don't care that much one way or another.
Thanks,
Mathieu
>
>
> Thanks,
> Sasha
>
> > Thanks,
> >
> > Mathieu
> >
>
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH v3 01/17] hashtable: introduce a small and naive hashtable
From: Sasha Levin @ 2012-08-28 11:27 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Tejun Heo, torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, rostedt-nx8X9YLhiw1AfugRpC6u6w,
mingo-X9Un+BFzKDI, ebiederm-aS9lmoZGLiVWk0Htik3J/w,
aarcange-H+wXaHxf7aLQT0dZR+AlfA, ericvh-Re5JQEeQqe8AvxtiuMwx3w,
netdev-u79uwXL29TY76Z2rM5mHXA, josh-iaAMLnmF4UmaiuxdJuQwMA,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w, axboe-tSWWG44O7X1aa/9Udqfwiw,
agk-H+wXaHxf7aLQT0dZR+AlfA, dm-devel-H+wXaHxf7aLQT0dZR+AlfA,
neilb-l3A5Bk7waGM, ccaulfie-H+wXaHxf7aLQT0dZR+AlfA,
teigland-H+wXaHxf7aLQT0dZR+AlfA,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw, fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
jesse-l0M0P4e3n4LQT0dZR+AlfA,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ejt-H+wXaHxf7aLQT0dZR+AlfA, snitzer-H+wXaHxf7aLQT0dZR+AlfA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
dev-yBygre7rU0TnMu66kgdUjQ, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
lw-BthXqXjhjHXQFUHtdCDX3A
In-Reply-To: <20120828101148.GA21683@Krystal>
On 08/28/2012 12:11 PM, Mathieu Desnoyers wrote:
> * Sasha Levin (levinsasha928-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org) wrote:
>> On 08/25/2012 06:24 AM, Mathieu Desnoyers wrote:
>>> * Tejun Heo (tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org) wrote:
>>>> Hello,
>>>>
>>>> On Sat, Aug 25, 2012 at 12:59:25AM +0200, Sasha Levin wrote:
>>>>> Thats the thing, the amount of things of things you can do with a given bucket
>>>>> is very limited. You can't add entries to any point besides the head (without
>>>>> walking the entire list).
>>>>
>>>> Kinda my point. We already have all the hlist*() interface to deal
>>>> with such cases. Having something which is evidently the trivial
>>>> hlist hashtable and advertises as such in the interface can be
>>>> helpful. I think we need that more than we need anything fancy.
>>>>
>>>> Heh, this is a debate about which one is less insignificant. I can
>>>> see your point. I'd really like to hear what others think on this.
>>>>
>>>> Guys, do we want something which is evidently trivial hlist hashtable
>>>> which can use hlist_*() API directly or do we want something better
>>>> encapsulated?
>>>
>>> My 2 cents, FWIW: I think this specific effort should target a trivially
>>> understandable API and implementation, for use-cases where one would be
>>> tempted to reimplement his own trivial hash table anyway. So here
>>> exposing hlist internals, with which kernel developers are already
>>> familiar, seems like a good approach in my opinion, because hiding stuff
>>> behind new abstraction might make the target users go away.
>>>
>>> Then, as we see the need, we can eventually merge a more elaborate hash
>>> table with poneys and whatnot, but I would expect that the trivial hash
>>> table implementation would still be useful. There are of course very
>>> compelling reasons to use a more featureful hash table: automatic
>>> resize, RT-aware updates, scalable updates, etc... but I see a purpose
>>> for a trivial implementation. Its primary strong points being:
>>>
>>> - it's trivially understandable, so anyone how want to be really sure
>>> they won't end up debugging the hash table instead of their
>>> work-in-progress code can have a full understanding of it,
>>> - it has few dependencies, which makes it easier to understand and
>>> easier to use in some contexts (e.g. early boot).
>>>
>>> So I'm in favor of not overdoing the abstraction for this trivial hash
>>> table, and honestly I would rather prefer that this trivial hash table
>>> stays trivial. A more elaborate hash table should probably come as a
>>> separate API.
>>>
>>> Thanks,
>>>
>>> Mathieu
>>>
>>
>> Alright, let's keep it simple then.
>>
>> I do want to keep the hash_for_each[rcu,safe] family though.
>
> Just a thought: if the API offered by the simple hash table focus on
> providing a mechanism to find the hash bucket to which belongs the hash
> chain containing the key looked up, and then expects the user to use the
> hlist API to iterate on the chain (with or without the hlist _rcu
> variant), then it might seem consistent that a helper providing
> iteration over the entire table would actually just provide iteration on
> all buckets, and let the user call the hlist for each iterator for each
> node within the bucket, e.g.:
>
> struct hlist_head *head;
> struct hlist_node *pos;
>
> hash_for_each_bucket(ht, head) {
> hlist_for_each(pos, head) {
> ...
> }
> }
>
> That way you only have to provide one single macro
> (hash_for_each_bucket), and rely on the already existing:
>
> - hlist_for_each_entry
> - hlist_for_each_safe
> - hlist_for_each_entry_rcu
> - hlist_for_each_safe_rcu
> .....
>
> and various flavors that can appear in the future without duplicating
> this API. So you won't even have to create _rcu, _safe, nor _safe_rcu
> versions of the hash_for_each_bucket macro.
>
> Thoughts ?
In my opinion, the downside here is that it'll require 2 function calls and 2
levels of nesting for a simple hash iteration.
hash_for_each_bucket() will always be followed by an iteration of that bucket,
so splitting a hash_for_each() which does both into 2 different functions which
will almost always must be called in that given order sounds unintuitive to me.
It's also just 3 different possible iterators:
- hlist_for_each_entry
- hlist_for_each_entry_safe
- hlist_for_each_entry_rcu
So I think that it's a good price to pay - 2 extra macro definitions in the
header to save a macro call + nesting level in each place that uses a hashtable.
Thanks,
Sasha
> Thanks,
>
> Mathieu
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: CBQ(but probably u32 filter bug), kernel "freeze", at least from 3.2.0
From: Eric Dumazet @ 2012-08-28 10:59 UTC (permalink / raw)
To: Denys Fedoryshchenko; +Cc: netdev
In-Reply-To: <a680a8297b2f099eb713b266792b9b82@visp.net.lb>
On Tue, 2012-08-28 at 07:50 +0300, Denys Fedoryshchenko wrote:
> Hi
>
> Got information from friend, confirmed that it crashed at least two my
> boxes also :)
> 3.0.5-rc1 is working fine, 3.4.1 , 3.2.0 from ubuntu - crashing
> No watchdog fired, and didn't got yet significant debugging
> information.
>
> Very easy to reproduce:
> 1)run the script
> 2)ping 192.168.3.234
>
> script:
> DEV_OUT=eth0
> ICMP="match ip protocol 1 0xff"
> U32="protocol ip u32"
> DST="match ip dst"
> tc qdisc add dev $DEV_OUT root handle 1: cbq avpkt 1000 bandwidth
> 100mbit
> tc class add dev $DEV_OUT parent 1: classid 1:1 cbq rate 512kbit allot
> 1500 prio 5 bounded isolated
> tc filter add dev $DEV_OUT parent 1: prio 3 $U32 $ICMP
> $DST 192.168.3.234 flowid 1:
> tc qdisc add dev $DEV_OUT parent 1:1 sfq perturb 10
Not sure what your friend expected from this buggy configuration.
It probably never worked at all.
CBQ needs at least one child class and one leaf class.
This scripts creates a loop inside CBQ, so cpu is probably looping in
cbq_enqueue() (or more exactly cbq_classify()), as instructed by the
sysadmin ;)
u32 (or sfq) seems ok.
Could you try the following patch ?
Thanks !
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index 6aabd77..564b9fc 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -250,10 +250,11 @@ cbq_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)
else if ((cl = defmap[res.classid & TC_PRIO_MAX]) == NULL)
cl = defmap[TC_PRIO_BESTEFFORT];
- if (cl == NULL || cl->level >= head->level)
+ if (cl == NULL)
goto fallback;
}
-
+ if (cl->level >= head->level)
+ goto fallback;
#ifdef CONFIG_NET_CLS_ACT
switch (result) {
case TC_ACT_QUEUED:
^ permalink raw reply related
* Re: [PATCH v3 01/17] hashtable: introduce a small and naive hashtable
From: Mathieu Desnoyers @ 2012-08-28 10:11 UTC (permalink / raw)
To: Sasha Levin
Cc: Tejun Heo, torvalds, akpm, linux-kernel, linux-mm, paul.gortmaker,
davem, rostedt, mingo, ebiederm, aarcange, ericvh, netdev, josh,
eric.dumazet, axboe, agk, dm-devel, neilb, ccaulfie, teigland,
Trond.Myklebust, bfields, fweisbec, jesse, venkat.x.venkatsubra,
ejt, snitzer, edumazet, linux-nfs, dev, rds-devel, lw
In-Reply-To: <503C95E4.3010000@gmail.com>
* Sasha Levin (levinsasha928@gmail.com) wrote:
> On 08/25/2012 06:24 AM, Mathieu Desnoyers wrote:
> > * Tejun Heo (tj@kernel.org) wrote:
> >> Hello,
> >>
> >> On Sat, Aug 25, 2012 at 12:59:25AM +0200, Sasha Levin wrote:
> >>> Thats the thing, the amount of things of things you can do with a given bucket
> >>> is very limited. You can't add entries to any point besides the head (without
> >>> walking the entire list).
> >>
> >> Kinda my point. We already have all the hlist*() interface to deal
> >> with such cases. Having something which is evidently the trivial
> >> hlist hashtable and advertises as such in the interface can be
> >> helpful. I think we need that more than we need anything fancy.
> >>
> >> Heh, this is a debate about which one is less insignificant. I can
> >> see your point. I'd really like to hear what others think on this.
> >>
> >> Guys, do we want something which is evidently trivial hlist hashtable
> >> which can use hlist_*() API directly or do we want something better
> >> encapsulated?
> >
> > My 2 cents, FWIW: I think this specific effort should target a trivially
> > understandable API and implementation, for use-cases where one would be
> > tempted to reimplement his own trivial hash table anyway. So here
> > exposing hlist internals, with which kernel developers are already
> > familiar, seems like a good approach in my opinion, because hiding stuff
> > behind new abstraction might make the target users go away.
> >
> > Then, as we see the need, we can eventually merge a more elaborate hash
> > table with poneys and whatnot, but I would expect that the trivial hash
> > table implementation would still be useful. There are of course very
> > compelling reasons to use a more featureful hash table: automatic
> > resize, RT-aware updates, scalable updates, etc... but I see a purpose
> > for a trivial implementation. Its primary strong points being:
> >
> > - it's trivially understandable, so anyone how want to be really sure
> > they won't end up debugging the hash table instead of their
> > work-in-progress code can have a full understanding of it,
> > - it has few dependencies, which makes it easier to understand and
> > easier to use in some contexts (e.g. early boot).
> >
> > So I'm in favor of not overdoing the abstraction for this trivial hash
> > table, and honestly I would rather prefer that this trivial hash table
> > stays trivial. A more elaborate hash table should probably come as a
> > separate API.
> >
> > Thanks,
> >
> > Mathieu
> >
>
> Alright, let's keep it simple then.
>
> I do want to keep the hash_for_each[rcu,safe] family though.
Just a thought: if the API offered by the simple hash table focus on
providing a mechanism to find the hash bucket to which belongs the hash
chain containing the key looked up, and then expects the user to use the
hlist API to iterate on the chain (with or without the hlist _rcu
variant), then it might seem consistent that a helper providing
iteration over the entire table would actually just provide iteration on
all buckets, and let the user call the hlist for each iterator for each
node within the bucket, e.g.:
struct hlist_head *head;
struct hlist_node *pos;
hash_for_each_bucket(ht, head) {
hlist_for_each(pos, head) {
...
}
}
That way you only have to provide one single macro
(hash_for_each_bucket), and rely on the already existing:
- hlist_for_each_entry
- hlist_for_each_safe
- hlist_for_each_entry_rcu
- hlist_for_each_safe_rcu
.....
and various flavors that can appear in the future without duplicating
this API. So you won't even have to create _rcu, _safe, nor _safe_rcu
versions of the hash_for_each_bucket macro.
Thoughts ?
Thanks,
Mathieu
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH v3 01/17] hashtable: introduce a small and naive hashtable
From: Sasha Levin @ 2012-08-28 9:56 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Tejun Heo, torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
paul.gortmaker-CWA4WttNNZF54TAoqtyWWQ,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, rostedt-nx8X9YLhiw1AfugRpC6u6w,
mingo-X9Un+BFzKDI, ebiederm-aS9lmoZGLiVWk0Htik3J/w,
aarcange-H+wXaHxf7aLQT0dZR+AlfA, ericvh-Re5JQEeQqe8AvxtiuMwx3w,
netdev-u79uwXL29TY76Z2rM5mHXA, josh-iaAMLnmF4UmaiuxdJuQwMA,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w, axboe-tSWWG44O7X1aa/9Udqfwiw,
agk-H+wXaHxf7aLQT0dZR+AlfA, dm-devel-H+wXaHxf7aLQT0dZR+AlfA,
neilb-l3A5Bk7waGM, ccaulfie-H+wXaHxf7aLQT0dZR+AlfA,
teigland-H+wXaHxf7aLQT0dZR+AlfA,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
bfields-uC3wQj2KruNg9hUCZPvPmw, fweisbec-Re5JQEeQqe8AvxtiuMwx3w,
jesse-l0M0P4e3n4LQT0dZR+AlfA,
venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA,
ejt-H+wXaHxf7aLQT0dZR+AlfA, snitzer-H+wXaHxf7aLQT0dZR+AlfA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
dev-yBygre7rU0TnMu66kgdUjQ, rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
lw-BthXqXjhjHXQFUHtdCDX3A
In-Reply-To: <20120825042419.GA27240@Krystal>
On 08/25/2012 06:24 AM, Mathieu Desnoyers wrote:
> * Tejun Heo (tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org) wrote:
>> Hello,
>>
>> On Sat, Aug 25, 2012 at 12:59:25AM +0200, Sasha Levin wrote:
>>> Thats the thing, the amount of things of things you can do with a given bucket
>>> is very limited. You can't add entries to any point besides the head (without
>>> walking the entire list).
>>
>> Kinda my point. We already have all the hlist*() interface to deal
>> with such cases. Having something which is evidently the trivial
>> hlist hashtable and advertises as such in the interface can be
>> helpful. I think we need that more than we need anything fancy.
>>
>> Heh, this is a debate about which one is less insignificant. I can
>> see your point. I'd really like to hear what others think on this.
>>
>> Guys, do we want something which is evidently trivial hlist hashtable
>> which can use hlist_*() API directly or do we want something better
>> encapsulated?
>
> My 2 cents, FWIW: I think this specific effort should target a trivially
> understandable API and implementation, for use-cases where one would be
> tempted to reimplement his own trivial hash table anyway. So here
> exposing hlist internals, with which kernel developers are already
> familiar, seems like a good approach in my opinion, because hiding stuff
> behind new abstraction might make the target users go away.
>
> Then, as we see the need, we can eventually merge a more elaborate hash
> table with poneys and whatnot, but I would expect that the trivial hash
> table implementation would still be useful. There are of course very
> compelling reasons to use a more featureful hash table: automatic
> resize, RT-aware updates, scalable updates, etc... but I see a purpose
> for a trivial implementation. Its primary strong points being:
>
> - it's trivially understandable, so anyone how want to be really sure
> they won't end up debugging the hash table instead of their
> work-in-progress code can have a full understanding of it,
> - it has few dependencies, which makes it easier to understand and
> easier to use in some contexts (e.g. early boot).
>
> So I'm in favor of not overdoing the abstraction for this trivial hash
> table, and honestly I would rather prefer that this trivial hash table
> stays trivial. A more elaborate hash table should probably come as a
> separate API.
>
> Thanks,
>
> Mathieu
>
Alright, let's keep it simple then.
I do want to keep the hash_for_each[rcu,safe] family though.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/2] ipvs: Extend MTU check to account for IPv6 NAT defrag changes
From: Julian Anastasov @ 2012-08-28 9:47 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, Patrick McHardy, lvs-devel, Simon Horman,
Pablo Neira Ayuso, Hans Schillstrom, Wensong Zhang,
netfilter-devel
In-Reply-To: <1346144607.3069.415.camel@localhost>
Hello,
On Tue, 28 Aug 2012, Jesper Dangaard Brouer wrote:
> On Mon, 2012-08-27 at 18:20 +0300, Julian Anastasov wrote:
> >
> > > @@ -956,8 +963,11 @@ ip_vs_tunnel_xmit_v6(struct sk_buff *skb, struct ip_vs_conn *cp,
> > > skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
> > >
> > > /* MTU checking: Special for tunnel mode */
> > > - if (mtu < ntohs(old_iph->payload_len) + sizeof(struct ipv6hdr) &&
>
> I guess:
> ntohs(old_iph->payload_len) + sizeof(struct ipv6hdr)
> Is the same as:
> skb->len
I think so. You can think of this in different way:
all transmitters are called from same place, there is no
difference in the packets we see. When we can use
__mtu_check_toobig_v6 for other methods relying on skb->len
being correct, we can do the same for tunnels, only that
tunnels have lower MTU, that is the only difference.
> > > - !skb_is_gso(skb)) {
> > > + if ((!IP6CB(skb)->frag_max_size &&
> > > + (mtu < ntohs(old_iph->payload_len) + sizeof(struct ipv6hdr) &&
> > > + !skb_is_gso(skb)))
> > > + || IP6CB(skb)->frag_max_size + sizeof(struct ipv6hdr) > mtu) {
>
> >
> > mtu is already reduced with the new outer header size,
> > may be we can just call __mtu_check_toobig_v6 with mtu?
>
> To Julian, is the extra sizeof(struct ipv6hdr) addition to
> frag_max_size, wrong? (as the mtu is already reduced)
Yes, sizeof(struct ipv6hdr) is needed only together
with payload_len because payload_len does not include the
first header.
> If above statements hold, I think we can simply use
> __mtu_check_toobig_v6() also for the tunnel case :-)
Yep
> --Jesper
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply
* Re: [PATCH] userns: Add basic quota support v2
From: Boaz Harrosh @ 2012-08-28 9:44 UTC (permalink / raw)
To: Jan Kara
Cc: Eric W. Biederman, linux-kernel, netdev, linux-fsdevel,
Serge E. Hallyn, David Miller, Steven Whitehouse, Mark Fasheh,
Joel Becker, Ben Myers, Alex Elder, Dmitry Monakhov, Abhijith Das
In-Reply-To: <20120828090544.GC5146@quack.suse.cz>
On 08/28/2012 12:05 PM, Jan Kara wrote:
>> +static inline u32 from_qown(struct user_namespace *user_ns, struct qown qown)
>> +{
>> + switch (qown.type) {
>> + case USRQUOTA:
>> + return from_kuid(user_ns, qown.uid);
>> + case GRPQUOTA:
>> + return from_kgid(user_ns, qown.gid);
>> + case XQM_PRJQUOTA:
>> + return (user_ns == &init_user_ns) ? qown.prj : -1;
>> + default:
>> + BUG();
>> + }
>> +}
> I would like a bit more if the function somehow expressed in its name
> that it returns id. id_from_qown() might be a bit too long given how often
> it is used. qown2id() would be OK but it would be inconsistent with how
> names of other functions you've added are formed. So I'm somewhat
> undecided...
>
qown_id()
> Honza
^ 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