* Re: [PATCH] act_mirred: don't go back.
From: jamal @ 2009-11-09 7:22 UTC (permalink / raw)
To: xiaosuo; +Cc: Stephen Hemminger, David S. Miller, netdev
In-Reply-To: <4AF7B9F4.6010507@gmail.com>
On Mon, 2009-11-09 at 14:43 +0800, Changli Gao wrote:
> don't go back.
I didnt follow the motivation for the patch. Is it a bug
fix for some setup? I may be too jet-lagged and i am missing
the functional difference vs what was already there.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH] check the return value of ndo_select_queue()
From: David Miller @ 2009-11-09 7:26 UTC (permalink / raw)
To: xiaosuo; +Cc: netdev
In-Reply-To: <4AF7C1FD.8000302@gmail.com>
From: Changli Gao <xiaosuo@gmail.com>
Date: Mon, 09 Nov 2009 15:17:17 +0800
> 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>
Make it a WARN() so that it ends up in kerneloops.org
^ permalink raw reply
* Re: [PATCH] act_mirred: don't go back.
From: Changli Gao @ 2009-11-09 8:31 UTC (permalink / raw)
To: hadi; +Cc: Stephen Hemminger, David S. Miller, netdev
In-Reply-To: <1257751376.8009.2.camel@bigi>
On Mon, Nov 9, 2009 at 3:22 PM, jamal <hadi@cyberus.ca> wrote:
> On Mon, 2009-11-09 at 14:43 +0800, Changli Gao wrote:
>> don't go back.
>
> I didnt follow the motivation for the patch. Is it a bug
> fix for some setup? I may be too jet-lagged and i am missing
> the functional difference vs what was already there.
>
code cleanup! :)
--
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 8:44 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20091108.232647.47855733.davem@davemloft.net>
On Mon, Nov 9, 2009 at 3:26 PM, David Miller <davem@davemloft.net> wrote:
> From: Changli Gao <xiaosuo@gmail.com>
> Date: Mon, 09 Nov 2009 15:17:17 +0800
>
>> 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>
>
> Make it a WARN() so that it ends up in kerneloops.org
>
Like this?
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);
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH] check the return value of ndo_select_queue()
From: David Miller @ 2009-11-09 9:13 UTC (permalink / raw)
To: xiaosuo; +Cc: netdev
In-Reply-To: <412e6f7f0911090044v33c331edg6096c0f9b9db4143@mail.gmail.com>
From: Changli Gao <xiaosuo@gmail.com>
Date: Mon, 9 Nov 2009 16:44:05 +0800
> On Mon, Nov 9, 2009 at 3:26 PM, David Miller <davem@davemloft.net> wrote:
>> From: Changli Gao <xiaosuo@gmail.com>
>> Date: Mon, 09 Nov 2009 15:17:17 +0800
>>
>>> 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>
>>
>> Make it a WARN() so that it ends up in kerneloops.org
>>
> Like this?
> 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);
Yes, something like that.
^ permalink raw reply
* [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
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