* [PATCH] tcp: provide more information on the tcp receive_queue bugs
From: Ilpo Järvinen @ 2009-11-09 9:19 UTC (permalink / raw)
To: David Miller; +Cc: Herbert Xu, Arjan van de Ven, Netdev
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2300 bytes --]
The addition of rcv_nxt allows to discern whether the skb
was out of place or tp->copied. Also catch fancy combination
of flags if necessary (sadly we might miss the actual causer
flags as it might have already returned).
Btw, we perhaps would want to forward copied_seq in
somewhere or otherwise we might have some nice loop with
WARN stuff within but where to do that safely I don't
know at this stage until more is known (but it is not
made significantly worse by this patch).
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
---
net/ipv4/tcp.c | 19 ++++++++++++-------
1 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 98440ad..f1813bc 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1183,7 +1183,9 @@ void tcp_cleanup_rbuf(struct sock *sk, int copied)
#if TCP_DEBUG
struct sk_buff *skb = skb_peek(&sk->sk_receive_queue);
- WARN_ON(skb && !before(tp->copied_seq, TCP_SKB_CB(skb)->end_seq));
+ WARN(skb && !before(tp->copied_seq, TCP_SKB_CB(skb)->end_seq),
+ KERN_INFO "cleanup rbuf bug: copied %X seq %X rcvnxt %X\n",
+ tp->copied_seq, TCP_SKB_CB(skb)->end_seq, tp->rcv_nxt);
#endif
if (inet_csk_ack_scheduled(sk)) {
@@ -1430,11 +1432,13 @@ int tcp_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
/* Now that we have two receive queues this
* shouldn't happen.
*/
- if (before(*seq, TCP_SKB_CB(skb)->seq)) {
- printk(KERN_INFO "recvmsg bug: copied %X "
- "seq %X\n", *seq, TCP_SKB_CB(skb)->seq);
+ if (WARN(before(*seq, TCP_SKB_CB(skb)->seq),
+ KERN_INFO "recvmsg bug: copied %X "
+ "seq %X rcvnxt %X fl %X\n", *seq,
+ TCP_SKB_CB(skb)->seq, tp->rcv_nxt,
+ flags))
break;
- }
+
offset = *seq - TCP_SKB_CB(skb)->seq;
if (tcp_hdr(skb)->syn)
offset--;
@@ -1443,8 +1447,9 @@ int tcp_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
if (tcp_hdr(skb)->fin)
goto found_fin_ok;
WARN(!(flags & MSG_PEEK), KERN_INFO "recvmsg bug 2: "
- "copied %X seq %X\n", *seq,
- TCP_SKB_CB(skb)->seq);
+ "copied %X seq %X rcvnxt %X fl %X\n",
+ *seq, TCP_SKB_CB(skb)->seq,
+ tp->rcv_nxt, flags);
}
/* Well, if we have backlog, try to process it now yet. */
--
1.5.6.5
^ permalink raw reply related
* [PATCH] check the return value of ndo_select_queue()
From: Changli Gao @ 2009-11-09 9:34 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, xiaosuo
check the return value of ndo_select_queue()
Check the return value of ndo_select_queue(). If the value isn't smaller
than the real_num_tx_queues, print a warning message, and reset it to zero.
Signed-off-by: Changli Gao <xiaosuo@gmail.com>
----
net/core/dev.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index b8f74cf..0a6bf2e 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1794,10 +1794,17 @@ static struct netdev_queue *dev_pick_tx(struct net_device *dev,
const struct net_device_ops *ops = dev->netdev_ops;
u16 queue_index = 0;
- if (ops->ndo_select_queue)
+ if (ops->ndo_select_queue) {
queue_index = ops->ndo_select_queue(dev, skb);
- else if (dev->real_num_tx_queues > 1)
+ if (queue_index >= dev->real_num_tx_queues) {
+ WARN(1, "%s selects TX queue %d, "
+ "but real number of TX queues is %d\n",
+ dev->name, queue_index, dev->real_num_tx_queues);
+ queue_index = 0;
+ }
+ } else if (dev->real_num_tx_queues > 1) {
queue_index = skb_tx_hash(dev, skb);
+ }
skb_set_queue_mapping(skb, queue_index);
return netdev_get_tx_queue(dev, queue_index);
^ permalink raw reply related
* Re: [PATCH] check the return value of ndo_select_queue()
From: Eric Dumazet @ 2009-11-09 9:50 UTC (permalink / raw)
To: xiaosuo; +Cc: David S. Miller, netdev
In-Reply-To: <4AF7E221.2040901@gmail.com>
Changli Gao a écrit :
> check the return value of ndo_select_queue()
>
> Check the return value of ndo_select_queue(). If the value isn't smaller
> than the real_num_tx_queues, print a warning message, and reset it to zero.
>
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ----
> net/core/dev.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index b8f74cf..0a6bf2e 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1794,10 +1794,17 @@ static struct netdev_queue *dev_pick_tx(struct net_device *dev,
> const struct net_device_ops *ops = dev->netdev_ops;
> u16 queue_index = 0;
>
> - if (ops->ndo_select_queue)
> + if (ops->ndo_select_queue) {
> queue_index = ops->ndo_select_queue(dev, skb);
> - else if (dev->real_num_tx_queues > 1)
> + if (queue_index >= dev->real_num_tx_queues) {
> + WARN(1, "%s selects TX queue %d, "
> + "but real number of TX queues is %d\n",
> + dev->name, queue_index, dev->real_num_tx_queues);
> + queue_index = 0;
> + }
> + } else if (dev->real_num_tx_queues > 1) {
> queue_index = skb_tx_hash(dev, skb);
> + }
>
> skb_set_queue_mapping(skb, queue_index);
> return netdev_get_tx_queue(dev, queue_index);
Yes, but this is a _very_ unlikely condition (a bug IMHO), so you could use :
if (unlikely(queue_index >= dev->real_num_tx_queues)) {
...
}
(This unlikely() clause is implied in WARN... macros)
^ permalink raw reply
* Re: [PATCH] check the return value of ndo_select_queue()
From: Eric Dumazet @ 2009-11-09 10:02 UTC (permalink / raw)
To: xiaosuo; +Cc: David S. Miller, netdev
In-Reply-To: <4AF7E5F1.6080608@gmail.com>
Eric Dumazet a écrit :
>
> Yes, but this is a _very_ unlikely condition (a bug IMHO), so you could use :
>
> if (unlikely(queue_index >= dev->real_num_tx_queues)) {
> ...
> }
>
> (This unlikely() clause is implied in WARN... macros)
>
Also, you want to trigger this message only once, or ratelimit it at least
^ permalink raw reply
* Re: [PATCH 2/2 net-next-2.6] au1000-eth: convert to platform_driver model
From: Florian Fainelli @ 2009-11-09 10:18 UTC (permalink / raw)
To: David Miller; +Cc: linux-mips, ralf, netdev
In-Reply-To: <20091108.210236.247950385.davem@davemloft.net>
Hi David, Ralf,
On Monday 09 November 2009 06:02:36 David Miller wrote:
> From: Florian Fainelli <florian@openwrt.org>
> Date: Sun, 8 Nov 2009 15:42:11 +0100
>
> > This patch converts the au1000-eth driver to become a full
> > platform-driver as it ought to be. We now pass PHY-speficic
> > configurations through platform_data but for compatibility
> > the driver still assumes the default settings (search for PHY1 on
> > MAC0) when no platform_data is passed. Tested on my MTX-1 board.
> >
> > Signed-off-by: Florian Fainelli <florian@openwrt.org>
>
> Ralf, feel free to merge this yourself since it depends upon
> the previous Alchemy platform patch:
>
> Acked-by: David S. Miller <davem@davemloft.net>
Thank you David. I will do a follow up patch which cleans up the driver once
that one gets merged.
^ permalink raw reply
* Re: [RFC] netlink: add socket destruction notification
From: Johannes Berg @ 2009-11-09 10:22 UTC (permalink / raw)
To: Patrick McHardy; +Cc: netdev, Jouni Malinen, Thomas Graf
In-Reply-To: <4AF442C2.9040704@trash.net>
[-- Attachment #1: Type: text/plain, Size: 1239 bytes --]
On Fri, 2009-11-06 at 16:37 +0100, Patrick McHardy wrote:
> >> This seems pretty similar to the NETLINK_URELEASE notifier invoked
> >> in netlink_release(). Wouldn't that one work as well?
> >
> > Hmm, it does seem similar, thanks for pointing it out. What exactly does
> > the condition
> > if (nlk->pid && !nlk->subscriptions) {
> >
> > mean though?
>
> nlk->pid is non-zero for bound sockets, which is basically any
> non-kernel socket which has either sent a message or explicitly
> called bind(). nlk->subscriptions is zero for sockets not bound
> to multicast groups.
>
> So effectively it invokes the notifier for all bound unicast
> userspace sockets. Not sure why it doesn't invoke the notifier
> for sockets that are used for both unicast and multicast
> reception. If that is a problem I think the second condition
> could be removed.
Thanks for the explanation. I think we'd need the second condition
removed, I don't see a reason to force a socket to not also have
multicast RX if it's used for any of the purposes we're looking at this
for. Guess we need to audit the callees to determine whether that's ok.
Can you quickly explain the difference between release and destruct?
johannes
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: query: net-next section mismatch(es)
From: Andi Kleen @ 2009-11-09 10:32 UTC (permalink / raw)
To: William Allen Simpson; +Cc: David Miller, netdev
In-Reply-To: <4AF777FC.4080805@gmail.com>
William Allen Simpson <william.allen.simpson@gmail.com> writes:
>
>
> But I did it with 'make vmlinux' anyway, still makes no sense to me:
You should report those to the respective maintainers of the code that throws
the warning (see MAINTAINERS) and to linux-kernel. This is not really related
to networking.
-Andi
--
ak@linux.intel.com -- Speaking for myself only.
^ permalink raw reply
* Re: [PATCH 1/8] udp: add a counter into udp_hslot
From: Andi Kleen @ 2009-11-09 10:39 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S. Miller, Linux Netdev List, Lucian Adrian Grijincu,
Octavian Purdila
In-Reply-To: <4AF72741.5070405@gmail.com>
Eric Dumazet <eric.dumazet@gmail.com> writes:
>
> +/**
> + * struct udp_hslot - UDP hash slot
> + *
> + * @head: head of list of sockets
> + * @count: number of sockets in 'head' list
> + * @lock: spinlock protecting changes to head/count
> + */
> struct udp_hslot {
> struct hlist_nulls_head head;
> + int count;
Do you really need an int? On 64bit it's free due to the alignment,
but on 32bit x86 it's costly and you blow up the table considerably,
increasing cache misses.
Again it would be nicer if that was a separate smaller table together
with the spinlock.
In theory could also put a short counter into the low level alignment
bits of the pointer and perhaps convert the spinlock to a bitlock?
Then all could collapse into a single pointer.
-Andi
--
ak@linux.intel.com -- Speaking for myself only.
^ permalink raw reply
* Re: [PATCH] act_mirred: don't go back.
From: jamal @ 2009-11-09 10:54 UTC (permalink / raw)
To: Changli Gao; +Cc: Stephen Hemminger, David S. Miller, netdev
In-Reply-To: <412e6f7f0911090031j16ab9c73o47f4d4d3ffd9be3a@mail.gmail.com>
On Mon, 2009-11-09 at 16:31 +0800, Changli Gao wrote:
> code cleanup! :)
I dont really see it as a cleanup to be honest. The code
is not less ugly even after the change;->
cheers,
jamal
^ permalink raw reply
* Re: [PATCH] act_mirred: don't go back.
From: Changli Gao @ 2009-11-09 11:06 UTC (permalink / raw)
To: hadi; +Cc: Stephen Hemminger, David S. Miller, netdev
In-Reply-To: <1257764086.6246.1.camel@bigi>
On Mon, Nov 9, 2009 at 6:54 PM, jamal <hadi@cyberus.ca> wrote:
> On Mon, 2009-11-09 at 16:31 +0800, Changli Gao wrote:
>
>> code cleanup! :)
>
> I dont really see it as a cleanup to be honest. The code
> is not less ugly even after the change;->
>
Maybe I should remove goto, I'll try later.
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH] check the return value of ndo_select_queue()
From: Changli Gao @ 2009-11-09 11:29 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S. Miller, netdev
In-Reply-To: <4AF7E8C2.90707@gmail.com>
2009/11/9 Eric Dumazet <eric.dumazet@gmail.com>:
> Eric Dumazet a écrit :
>>
>> Yes, but this is a _very_ unlikely condition (a bug IMHO), so you could use :
>>
>> if (unlikely(queue_index >= dev->real_num_tx_queues)) {
>> ...
>> }
>>
>> (This unlikely() clause is implied in WARN... macros)
>>
>
> Also, you want to trigger this message only once, or ratelimit it at least
>
>
How about this version:
u16 queue_index;
u16 (*ndo_select_queue)(struct net_device*, struct sk_buff*);
unsigned int real_num_tx_queues = dev->real_num_tx_queues;
if (real_num_tx_queues == 1) {
queue_index = 0;
} else if ((ndo_select_queue = dev->netdev_ops->ndo_select_queue)) {
queue_index = ndo_select_queue(dev, skb);
if (unlikely(queue_index >= real_num_tx_queues)) {
if (net_ratelimit())
WARN(1, "%s selects TX queue %d, "
"but real number of TX queues is %d\n",
dev->name, queue_index,
real_num_tx_queues);
queue_index = 0;
}
} else {
queue_index = skb_tx_hash(dev, skb);
}
skb_set_queue_mapping(skb, queue_index);
return netdev_get_tx_queue(dev, queue_index);
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: query: net-next section mismatch(es)
From: William Allen Simpson @ 2009-11-09 11:36 UTC (permalink / raw)
To: Andi Kleen; +Cc: David Miller, netdev
In-Reply-To: <87k4y056q2.fsf@basil.nowhere.org>
Andi Kleen wrote:
> You should report those to the respective maintainers of the code that throws
> the warning (see MAINTAINERS) and to linux-kernel. This is not really related
> to networking.
>
Thanks. I'll do that. I reported it here, as it was a fairly significant
change to net-next, and I assumed it was probably due to some recent problem
importing/merging some other branch into this one. But that's just a guess,
so it's phrased as a query....
^ permalink raw reply
* Re: [PATCH 1/8] udp: add a counter into udp_hslot
From: Eric Dumazet @ 2009-11-09 11:42 UTC (permalink / raw)
To: Andi Kleen
Cc: David S. Miller, Linux Netdev List, Lucian Adrian Grijincu,
Octavian Purdila
In-Reply-To: <87fx8o56eq.fsf@basil.nowhere.org>
Andi Kleen a écrit :
> Eric Dumazet <eric.dumazet@gmail.com> writes:
>>
>> +/**
>> + * struct udp_hslot - UDP hash slot
>> + *
>> + * @head: head of list of sockets
>> + * @count: number of sockets in 'head' list
>> + * @lock: spinlock protecting changes to head/count
>> + */
>> struct udp_hslot {
>> struct hlist_nulls_head head;
>> + int count;
>
> Do you really need an int? On 64bit it's free due to the alignment,
> but on 32bit x86 it's costly and you blow up the table considerably,
> increasing cache misses.
Even a short (16 bits) might be too small for IXIACOM :)
On 32bit x86, size of hash table is 512 slots max.
(one slot per 2MB of LOWMEM, rounded to power of two)
You are speaking of <= 4096 bytes overhead :)
>
> Again it would be nicer if that was a separate smaller table together
> with the spinlock.
Nice for space, not nice for fast path, because this means additional
cache miss to get the spinlock (multicast rx still needs to take spinlock),
and some guys want really fast (low latency) multicast rx.
>
> In theory could also put a short counter into the low level alignment
> bits of the pointer and perhaps convert the spinlock to a bitlock?
> Then all could collapse into a single pointer.
>
Not enough bits in low level alignment unfortunatly. We only could give a
hint (one bit is enough) of possibly long chain, but not allowing precise
choice of shortest chain.
Once multicast is converted to RCU, then we wont need one spinlock per slot
(it wont be used in fast path, only at bind()/close() time)
and yes, we can use a separate small array to contain hashed spinlocks,
or even a single spinlock for CONFIG_BASE_SMALL :)
^ permalink raw reply
* Re: [PATCHv8 3/3] vhost_net: a kernel-level virtio server
From: Michael S. Tsirkin @ 2009-11-09 11:55 UTC (permalink / raw)
To: Rusty Russell
Cc: netdev, virtualization, kvm, linux-kernel, mingo, linux-mm, akpm,
hpa, gregory.haskins, s.hetze, Daniel Walker, Eric Dumazet
In-Reply-To: <200911091647.29655.rusty@rustcorp.com.au>
On Mon, Nov 09, 2009 at 04:47:29PM +1030, Rusty Russell wrote:
> Actually, this looks wrong to me:
>
> + case VHOST_SET_VRING_BASE:
> ...
> + vq->avail_idx = vq->last_avail_idx = s.num;
>
> The last_avail_idx is part of the state of the driver. It needs to be saved
> and restored over susp/resume. The only reason it's not in the ring itself
> is because I figured the other side doesn't need to see it (which is true, but
> missed debugging opportunities as well as man-in-the-middle issues like this
> one). I had a patch which put this field at the end of the ring, I might
> resurrect it to avoid this problem. This is backwards compatible with all
> implementations. See patch at end.
>
> I would drop avail_idx altogether: get_user is basically free, and simplifies
> a lot. As most state is in the ring, all you need is an ioctl to save/restore
> the last_avail_idx.
I remembered another reason for caching head in avail_idx. Basically,
avail index could change between when I poll for descriptors and when I
want to notify guest.
So we could have:
- poll descriptors until empty
- notify
detects not empty so does not notify
And the way to solve it would be to return flag from
notify telling us to restart the polling loop.
But, this will be more code, on data path, than
what happens today where I simply keep state
from descriptor polling and use that to notify.
I also suspect that somehow this race in practice can not create
deadlocks ... but I prefer to avoid it, these things are very tricky: if
I see an empty ring, and stop processing descriptors, I want to trigger
notify on empty.
So if we want to avoid keeping "empty" state, IMO the best way would be
to pass a flag to vhost_signal that tells it that ring is empty.
Makes sense?
--
MST
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH 43/75] spider-net: declare MODULE_FIRMWARE
From: Jens Osterkamp @ 2009-11-09 11:58 UTC (permalink / raw)
To: Ben Hutchings; +Cc: David Miller, Ishizaki Kou, Jens Osterkamp, netdev, arnd
In-Reply-To: <1257630907.15927.445.camel@localhost>
On Saturday 07 November 2009, Ben Hutchings wrote:
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
> drivers/net/spider_net.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/spider_net.c b/drivers/net/spider_net.c
> index 90e663f..782910c 100644
> --- a/drivers/net/spider_net.c
> +++ b/drivers/net/spider_net.c
> @@ -57,6 +57,7 @@ MODULE_AUTHOR("Utz Bacher <utz.bacher@de.ibm.com> and
> Jens Osterkamp " \ MODULE_DESCRIPTION("Spider Southbridge Gigabit Ethernet
> driver"); MODULE_LICENSE("GPL");
> MODULE_VERSION(VERSION);
> +MODULE_FIRMWARE(SPIDER_NET_FIRMWARE_NAME);
>
> static int rx_descriptors = SPIDER_NET_RX_DESCRIPTORS_DEFAULT;
> static int tx_descriptors = SPIDER_NET_TX_DESCRIPTORS_DEFAULT;
In spider_net, the filesystem is checked for a valid firmware first, if it
does not exist, the firmware is taken from the device tree (which is the
default case).
Do you expect this behaviour to cause problems together with MODULE_FIRMWARE ?
--
Best regards,
Jens Osterkamp
--------------------------------------------------------------------------------
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschäftsführung: Erich Baier
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
^ permalink raw reply
* Re: [PATCH 1/8] udp: add a counter into udp_hslot
From: Andi Kleen @ 2009-11-09 12:10 UTC (permalink / raw)
To: Eric Dumazet
Cc: Andi Kleen, David S. Miller, Linux Netdev List,
Lucian Adrian Grijincu, Octavian Purdila
In-Reply-To: <4AF80040.6060501@gmail.com>
> > Do you really need an int? On 64bit it's free due to the alignment,
> > but on 32bit x86 it's costly and you blow up the table considerably,
> > increasing cache misses.
>
> Even a short (16 bits) might be too small for IXIACOM :)
True, but see below.
>
> On 32bit x86, size of hash table is 512 slots max.
> (one slot per 2MB of LOWMEM, rounded to power of two)
>
> You are speaking of <= 4096 bytes overhead :)
Well it's cache line overhead too. 32bit systems have often
small caches (i.e. Atom)
>
> >
> > Again it would be nicer if that was a separate smaller table together
> > with the spinlock.
>
> Nice for space, not nice for fast path, because this means additional
> cache miss to get the spinlock (multicast rx still needs to take spinlock),
> and some guys want really fast (low latency) multicast rx.
When the spinlock use is mostly local it should be in cache
(that's the nice thing about small tables, they don't drop out of cache)
>
> >
> > In theory could also put a short counter into the low level alignment
> > bits of the pointer and perhaps convert the spinlock to a bitlock?
> > Then all could collapse into a single pointer.
> >
>
> Not enough bits in low level alignment unfortunatly. We only could give a
> hint (one bit is enough) of possibly long chain, but not allowing precise
> choice of shortest chain.
Do we really need a precise answer here? I would assume an approximate
answer would be good enough using a saturating counter.
e.g. if both have >N just round robin.
Ok it would be tricky to decrement that again on unbind, but I assume just
continuing to RR later wouldn't be too bad.
The question is just if there are enough bits even for that.
> Once multicast is converted to RCU, then we wont need one spinlock per slot
> (it wont be used in fast path, only at bind()/close() time)
> and yes, we can use a separate small array to contain hashed spinlocks,
> or even a single spinlock for CONFIG_BASE_SMALL :)
Or a bit spinlock in the bucket low pointer bits.
With that (and the saturating counter) even the 64bit table could be shortened to half.
-Andi
--
ak@linux.intel.com -- Speaking for myself only.
^ permalink raw reply
* [PATCH net-next] Phonet: put sockets in a hash table
From: Rémi Denis-Courmont @ 2009-11-09 12:17 UTC (permalink / raw)
To: netdev; +Cc: Rémi Denis-Courmont
From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
include/net/phonet/phonet.h | 1 +
net/phonet/af_phonet.c | 1 +
net/phonet/socket.c | 79 +++++++++++++++++++++++++++++-------------
3 files changed, 56 insertions(+), 25 deletions(-)
diff --git a/include/net/phonet/phonet.h b/include/net/phonet/phonet.h
index fdb05fa..7b11407 100644
--- a/include/net/phonet/phonet.h
+++ b/include/net/phonet/phonet.h
@@ -46,6 +46,7 @@ static inline struct pn_sock *pn_sk(struct sock *sk)
extern const struct proto_ops phonet_dgram_ops;
+void pn_sock_init(void);
struct sock *pn_find_sock_by_sa(struct net *net, const struct sockaddr_pn *sa);
void pn_deliver_sock_broadcast(struct net *net, struct sk_buff *skb);
void phonet_get_local_port_range(int *min, int *max);
diff --git a/net/phonet/af_phonet.c b/net/phonet/af_phonet.c
index 3bd1be6..8d3a55b 100644
--- a/net/phonet/af_phonet.c
+++ b/net/phonet/af_phonet.c
@@ -481,6 +481,7 @@ static int __init phonet_init(void)
if (err)
return err;
+ pn_sock_init();
err = sock_register(&phonet_proto_family);
if (err) {
printk(KERN_ALERT
diff --git a/net/phonet/socket.c b/net/phonet/socket.c
index 0412beb..4112b6e 100644
--- a/net/phonet/socket.c
+++ b/net/phonet/socket.c
@@ -45,13 +45,28 @@ static int pn_socket_release(struct socket *sock)
return 0;
}
+#define PN_HASHSIZE 16
+#define PN_HASHMASK (PN_HASHSIZE-1)
+
+
static struct {
- struct hlist_head hlist;
+ struct hlist_head hlist[PN_HASHSIZE];
spinlock_t lock;
-} pnsocks = {
- .hlist = HLIST_HEAD_INIT,
- .lock = __SPIN_LOCK_UNLOCKED(pnsocks.lock),
-};
+} pnsocks;
+
+void __init pn_sock_init(void)
+{
+ unsigned i;
+
+ for (i = 0; i < PN_HASHSIZE; i++)
+ INIT_HLIST_HEAD(pnsocks.hlist + i);
+ spin_lock_init(&pnsocks.lock);
+}
+
+static struct hlist_head *pn_hash_list(u16 obj)
+{
+ return pnsocks.hlist + (obj & PN_HASHMASK);
+}
/*
* Find address based on socket address, match only certain fields.
@@ -64,10 +79,11 @@ struct sock *pn_find_sock_by_sa(struct net *net, const struct sockaddr_pn *spn)
struct sock *rval = NULL;
u16 obj = pn_sockaddr_get_object(spn);
u8 res = spn->spn_resource;
+ struct hlist_head *hlist = pn_hash_list(obj);
spin_lock_bh(&pnsocks.lock);
- sk_for_each(sknode, node, &pnsocks.hlist) {
+ sk_for_each(sknode, node, hlist) {
struct pn_sock *pn = pn_sk(sknode);
BUG_ON(!pn->sobject); /* unbound socket */
@@ -99,31 +115,39 @@ struct sock *pn_find_sock_by_sa(struct net *net, const struct sockaddr_pn *spn)
/* Deliver a broadcast packet (only in bottom-half) */
void pn_deliver_sock_broadcast(struct net *net, struct sk_buff *skb)
{
- struct hlist_node *node;
- struct sock *sknode;
+ struct hlist_head *hlist = pnsocks.hlist;
+ unsigned h;
spin_lock(&pnsocks.lock);
- sk_for_each(sknode, node, &pnsocks.hlist) {
- struct sk_buff *clone;
+ for (h = 0; h < PN_HASHSIZE; h++) {
+ struct hlist_node *node;
+ struct sock *sknode;
- if (!net_eq(sock_net(sknode), net))
- continue;
- if (!sock_flag(sknode, SOCK_BROADCAST))
- continue;
+ sk_for_each(sknode, node, hlist) {
+ struct sk_buff *clone;
- clone = skb_clone(skb, GFP_ATOMIC);
- if (clone) {
- sock_hold(sknode);
- sk_receive_skb(sknode, clone, 0);
+ if (!net_eq(sock_net(sknode), net))
+ continue;
+ if (!sock_flag(sknode, SOCK_BROADCAST))
+ continue;
+
+ clone = skb_clone(skb, GFP_ATOMIC);
+ if (clone) {
+ sock_hold(sknode);
+ sk_receive_skb(sknode, clone, 0);
+ }
}
+ hlist++;
}
spin_unlock(&pnsocks.lock);
}
void pn_sock_hash(struct sock *sk)
{
+ struct hlist_head *hlist = pn_hash_list(pn_sk(sk)->sobject);
+
spin_lock_bh(&pnsocks.lock);
- sk_add_node(sk, &pnsocks.hlist);
+ sk_add_node(sk, hlist);
spin_unlock_bh(&pnsocks.lock);
}
EXPORT_SYMBOL(pn_sock_hash);
@@ -439,15 +463,20 @@ EXPORT_SYMBOL(pn_sock_get_port);
static struct sock *pn_sock_get_idx(struct seq_file *seq, loff_t pos)
{
struct net *net = seq_file_net(seq);
+ struct hlist_head *hlist = pnsocks.hlist;
struct hlist_node *node;
struct sock *sknode;
+ unsigned h;
- sk_for_each(sknode, node, &pnsocks.hlist) {
- if (!net_eq(net, sock_net(sknode)))
- continue;
- if (!pos)
- return sknode;
- pos--;
+ for (h = 0; h < PN_HASHSIZE; h++) {
+ sk_for_each(sknode, node, hlist) {
+ if (!net_eq(net, sock_net(sknode)))
+ continue;
+ if (!pos)
+ return sknode;
+ pos--;
+ }
+ hlist++;
}
return NULL;
}
--
1.6.3.3
^ permalink raw reply related
* Re: [PATCH] act_mirred: don't go back.
From: Changli Gao @ 2009-11-09 12:33 UTC (permalink / raw)
To: hadi; +Cc: Stephen Hemminger, David S. Miller, netdev
In-Reply-To: <1257764086.6246.1.camel@bigi>
On Mon, Nov 9, 2009 at 6:54 PM, jamal <hadi@cyberus.ca> wrote:
> On Mon, 2009-11-09 at 16:31 +0800, Changli Gao wrote:
>
>> code cleanup! :)
>
> I dont really see it as a cleanup to be honest. The code
> is not less ugly even after the change;->
>
static int tcf_mirred(struct sk_buff *skb, struct tc_action *a,
struct tcf_result *res)
{
struct net_device *dev;
struct sk_buff *skb2;
u32 at;
struct tcf_mirred *m = a->priv;
int retval, err = 1;
spin_lock(&m->tcf_lock);
m->tcf_tm.lastuse = jiffies;
if (m->tcfm_eaction != TCA_EGRESS_MIRROR &&
m->tcfm_eaction != TCA_EGRESS_REDIR) {
if (net_ratelimit())
printk("tcf_mirred unknown action %d\n",
m->tcfm_eaction);
goto out;
}
dev = m->tcfm_dev;
if (!(dev->flags&IFF_UP) ) {
if (net_ratelimit())
printk("mirred to Houston: device %s is gone!\n",
dev->name);
goto out;
}
skb2 = skb_act_clone(skb, GFP_ATOMIC);
if (skb2 == NULL)
goto out;
m->tcf_bstats.bytes += qdisc_pkt_len(skb2);
m->tcf_bstats.packets++;
at = G_TC_AT(skb->tc_verd);
if (!(at & AT_EGRESS)) {
if (m->tcfm_ok_push)
skb_push(skb2, skb2->dev->hard_header_len);
}
/* mirror is always swallowed */
if (m->tcfm_eaction != TCA_EGRESS_MIRROR)
skb2->tc_verd = SET_TC_FROM(skb2->tc_verd, at);
skb2->dev = dev;
skb2->iif = skb->dev->ifindex;
dev_queue_xmit(skb2);
err = 0;
out:
if (err) {
m->tcf_qstats.overlimits++;
m->tcf_bstats.bytes += qdisc_pkt_len(skb);
m->tcf_bstats.packets++;
/* should we be asking for packet to be dropped?
* may make sense for redirect case only
*/
retval = TC_ACT_SHOT;
} else {
retval = m->tcf_action;
}
spin_unlock(&m->tcf_lock);
return retval;
}
How about this version.
1. move skb_act_clone() after all the necessary checks, and it can
eliminate unnecessary skb_act_clone() if tcfm_eaction isn't correct.
2. there is one exit of the critical section.
3. jump forward instead of backward.
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [RFC] netlink: add socket destruction notification
From: Patrick McHardy @ 2009-11-09 12:59 UTC (permalink / raw)
To: Johannes Berg; +Cc: netdev, Jouni Malinen, Thomas Graf
In-Reply-To: <1257762132.29454.161.camel@johannes.local>
Johannes Berg wrote:
> On Fri, 2009-11-06 at 16:37 +0100, Patrick McHardy wrote:
>
>>>> This seems pretty similar to the NETLINK_URELEASE notifier invoked
>>>> in netlink_release(). Wouldn't that one work as well?
>>> Hmm, it does seem similar, thanks for pointing it out. What exactly does
>>> the condition
>>> if (nlk->pid && !nlk->subscriptions) {
>>>
>>> mean though?
>> nlk->pid is non-zero for bound sockets, which is basically any
>> non-kernel socket which has either sent a message or explicitly
>> called bind(). nlk->subscriptions is zero for sockets not bound
>> to multicast groups.
>>
>> So effectively it invokes the notifier for all bound unicast
>> userspace sockets. Not sure why it doesn't invoke the notifier
>> for sockets that are used for both unicast and multicast
>> reception. If that is a problem I think the second condition
>> could be removed.
>
> Thanks for the explanation. I think we'd need the second condition
> removed, I don't see a reason to force a socket to not also have
> multicast RX if it's used for any of the purposes we're looking at this
> for. Guess we need to audit the callees to determine whether that's ok.
I've already done that. Its currently only used by netfilter
for which this change also makes sense.
> Can you quickly explain the difference between release and destruct?
release is called when the socket is closed, destruct is called
once all references are gone. I think with the synchonous processing
done nowadays they shouldn't make any difference, but release
should be fine in either case.
^ permalink raw reply
* Re: [RFC] netlink: add socket destruction notification
From: Johannes Berg @ 2009-11-09 13:03 UTC (permalink / raw)
To: Patrick McHardy; +Cc: netdev, Jouni Malinen, Thomas Graf
In-Reply-To: <4AF8122F.9060807@trash.net>
[-- Attachment #1: Type: text/plain, Size: 1239 bytes --]
On Mon, 2009-11-09 at 13:59 +0100, Patrick McHardy wrote:
> > Thanks for the explanation. I think we'd need the second condition
> > removed, I don't see a reason to force a socket to not also have
> > multicast RX if it's used for any of the purposes we're looking at this
> > for. Guess we need to audit the callees to determine whether that's ok.
>
> I've already done that. Its currently only used by netfilter
> for which this change also makes sense.
Cool, I arrived at that conclusion too, it seemed that it would
currently be somewhat strangely broken if you could add multicast groups
to those sockets used there. Not sure if you can though.
> > Can you quickly explain the difference between release and destruct?
>
> release is called when the socket is closed, destruct is called
> once all references are gone. I think with the synchonous processing
> done nowadays they shouldn't make any difference, but release
> should be fine in either case.
Ok, cool, thanks. Do you want me to send the change removing the
multicast check, or would you want to do that since you audited all the
netlink callers?
Also, it's called URELEASE for unicast -- should we rename it to just
RELEASE?
johannes
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [RFC] netlink: add socket destruction notification
From: Patrick McHardy @ 2009-11-09 13:19 UTC (permalink / raw)
To: Johannes Berg; +Cc: netdev, Jouni Malinen, Thomas Graf
In-Reply-To: <1257771787.29454.173.camel@johannes.local>
Johannes Berg wrote:
> On Mon, 2009-11-09 at 13:59 +0100, Patrick McHardy wrote:
>
>>> Thanks for the explanation. I think we'd need the second condition
>>> removed, I don't see a reason to force a socket to not also have
>>> multicast RX if it's used for any of the purposes we're looking at this
>>> for. Guess we need to audit the callees to determine whether that's ok.
>> I've already done that. Its currently only used by netfilter
>> for which this change also makes sense.
>
> Cool, I arrived at that conclusion too, it seemed that it would
> currently be somewhat strangely broken if you could add multicast groups
> to those sockets used there. Not sure if you can though.
I don't see anything preventing it.
>>> Can you quickly explain the difference between release and destruct?
>> release is called when the socket is closed, destruct is called
>> once all references are gone. I think with the synchonous processing
>> done nowadays they shouldn't make any difference, but release
>> should be fine in either case.
>
> Ok, cool, thanks. Do you want me to send the change removing the
> multicast check, or would you want to do that since you audited all the
> netlink callers?
Please go ahead.
> Also, it's called URELEASE for unicast -- should we rename it to just
> RELEASE?
I think URELEASE is still fine since won't necessarily get called
for sockets that are used for pure multicast reception when using
setsockopt to bind to groups. I also have a cleanup patch removing
unneccessary nlk->pid checks from netfilter which would clash with
a rename :)
^ permalink raw reply
* Re: [PATCH RFC] gianfar: Make polling safe with IRQs disabled
From: Anton Vorontsov @ 2009-11-09 13:32 UTC (permalink / raw)
To: David Miller; +Cc: afleming, jason.wessel, netdev, linuxppc-dev
In-Reply-To: <20091108.010533.212571001.davem@davemloft.net>
On Sun, Nov 08, 2009 at 01:05:33AM -0800, David Miller wrote:
...
> > When using KGDBoE, gianfar driver spits 'Interrupt problem' messages,
> > which appears to be a legitimate warning, i.e. we may end up calling
> > netif_receive_skb() or vlan_hwaccel_receive_skb() with IRQs disabled.
> >
> > This patch reworks the RX path so that if netpoll is enabled (the
> > only case when the driver don't know from what context the polling
> > may be called), we check whether IRQs are disabled, and if so we
> > fall back to safe variants of skb receiving functions.
>
> This is bogus, I'll tell you why.
>
> When you go into netif_receive_skb() we have a special check,
> "if (netpoll_receive_skb(..." that takes care of all of the
> details concerning doing a ->poll() from IRQ disabled context
> via netpoll.
>
> So this code you're adding should not be necessary.
>
> Or, explain to me why no other driver needs special logic in their
> ->poll() handler like this and does not run into any kinds of netpoll
> problems :-)
Hm, I was confused by the following note:
/**
* netif_receive_skb - process receive buffer from network
* @skb: buffer to process
...
* This function may only be called from softirq context and interrupts
* should be enabled.
Looking into the code though, I can indeed see that there
are netpoll checks, and __netpoll_rx() is actually called with
irqs disabled. So, in the end it appears that we should just
remove the 'Interrupt problem' message.
Thanks!
--
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
^ permalink raw reply
* Re: [PATCH RFC] gianfar: Do not call skb recycling with disabled IRQs
From: Anton Vorontsov @ 2009-11-09 13:41 UTC (permalink / raw)
To: David Miller
Cc: jdl, linuxppc-dev, jason.wessel, afleming, netdev, buytenh,
shemminger
In-Reply-To: <20091108.010848.116517157.davem@davemloft.net>
On Sun, Nov 08, 2009 at 01:08:48AM -0800, David Miller wrote:
> From: Anton Vorontsov <avorontsov@ru.mvista.com>
> Date: Thu, 5 Nov 2009 19:57:38 +0300
>
> > But that basically means that with skb recycling we can't safely
> > use KGDBoE, though we can add something like this:
>
> Please stop adding special logic only to your driver to handle these
> things.
>
> Either it's a non-issue, or it's going to potentially be an issue for
> everyone using skb_recycle_check() in a NAPI driver, right?
>
> So why not add the "in_interrupt()" or whatever check to
> skb_recycle_check() and if the context is unsuitable return false (0)
> ok?
I think the check is needed, yes. But if we add just that check,
then we'll never do skb recycling in the tx path (since gianfar
always grabs irqsave spinlock during tx ring cleanup, so the check
will always return false).
So we must add the check to keep polling with disabled IRQs safe,
but we also should get rid of irqsave spinlock in the gianfar
driver (to make the skb recycling actually work in most cases).
Thanks!
--
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
^ permalink raw reply
* [PATCH net-next] Phonet: allocate and copy for pipe TX without sock lock
From: Rémi Denis-Courmont @ 2009-11-09 14:06 UTC (permalink / raw)
To: netdev; +Cc: Rémi Denis-Courmont
From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
net/phonet/pep.c | 29 ++++++++++++-----------------
1 files changed, 12 insertions(+), 17 deletions(-)
diff --git a/net/phonet/pep.c b/net/phonet/pep.c
index cbaa1d6..bdc17bd 100644
--- a/net/phonet/pep.c
+++ b/net/phonet/pep.c
@@ -843,7 +843,7 @@ static int pep_sendmsg(struct kiocb *iocb, struct sock *sk,
struct msghdr *msg, size_t len)
{
struct pep_sock *pn = pep_sk(sk);
- struct sk_buff *skb = NULL;
+ struct sk_buff *skb;
long timeo;
int flags = msg->msg_flags;
int err, done;
@@ -851,6 +851,16 @@ static int pep_sendmsg(struct kiocb *iocb, struct sock *sk,
if (msg->msg_flags & MSG_OOB || !(msg->msg_flags & MSG_EOR))
return -EOPNOTSUPP;
+ skb = sock_alloc_send_skb(sk, MAX_PNPIPE_HEADER + len,
+ flags & MSG_DONTWAIT, &err);
+ if (!skb)
+ return -ENOBUFS;
+
+ skb_reserve(skb, MAX_PHONET_HEADER + 3);
+ err = memcpy_fromiovec(skb_put(skb, len), msg->msg_iov, len);
+ if (err < 0)
+ goto outfree;
+
lock_sock(sk);
timeo = sock_sndtimeo(sk, flags & MSG_DONTWAIT);
if ((1 << sk->sk_state) & (TCPF_LISTEN|TCPF_CLOSE)) {
@@ -894,28 +904,13 @@ disabled:
goto disabled;
}
- if (!skb) {
- skb = sock_alloc_send_skb(sk, MAX_PNPIPE_HEADER + len,
- flags & MSG_DONTWAIT, &err);
- if (skb == NULL)
- goto out;
- skb_reserve(skb, MAX_PHONET_HEADER + 3);
-
- if (sk->sk_state != TCP_ESTABLISHED ||
- !atomic_read(&pn->tx_credits))
- goto disabled; /* sock_alloc_send_skb might sleep */
- }
-
- err = memcpy_fromiovec(skb_put(skb, len), msg->msg_iov, len);
- if (err < 0)
- goto out;
-
err = pipe_skb_send(sk, skb);
if (err >= 0)
err = len; /* success! */
skb = NULL;
out:
release_sock(sk);
+outfree:
kfree_skb(skb);
return err;
}
--
1.6.3.3
^ permalink raw reply related
* RE: PATCH: Network Device Naming mechanism and policy
From: Narendra_K @ 2009-11-09 14:41 UTC (permalink / raw)
To: md, Matt_Domsch
Cc: bryan, dannf, bhutchings, netdev, linux-hotplug, Jordan_Hargrave,
Charles_Rose, Sandeep_K_Shandilya
In-Reply-To: <20091106223524.GA27121@bongo.bofh.it>
>> > As a distribution developer I highly value solutions like
>this which
>> > do not require patching every application which deals with
>interface
>> > names and then teaching users about aliases which only
>work in some
>> > places and are unknown to the kernel.
>> Fair enough - but would you object if we changed the naming scheme
>> from eth%d to something else?
>I suppose that this would depend on what else. :-) Since you
>want radical changes I recommend that you design the new
>persistent naming infrastructure in a way that will allow root
>to choose to use the classic naming scheme, or many users will
>scream a lot and at least some distributions will do it anyway.
>I also expect that providing choice at the beginning of
>development may lead to more acceptance later if and when the
>new scheme will have proved itself to be superior (at least in
>some situations).
>You have tought about this for a long time and if so far you
>have not found a solution which is widely considered superior
>then I doubt that one will appear soon. Providing your
>favourite naming scheme as an optional add on will immediately
>benefit those who like it and greatly reduce opposition from
>those who do not.
In that way, I suppose char device node solution fits the scheme
perfectly. It doesn't change or interfere with the kernel's default
naming scheme (ethN) in any way. Also, the applications continue to work
the way they did and in addition to supporting traditional names, they
would also support pathnames. Whether all the user space applications
need to be patched can be discussed and debated. But, we can patch
applications like, installers and firewall code, which when don't see
determinism ("eth0 mapping to integrated port 1"), fail and cause very
high impact could be patched. Since users are already familiar with
pathnames like /dev/disk/by-id{label, uuid}, I suppose it might not be
very difficult to get used to pathnames like
/dev/netdev/by-chassis-label/Embedded_NIC_1. Would that be acceptable ?
With regards,
Narendra K
^ 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