* Re: [PPATCH net-next-2.6] ipv4: inetdev_by_index() switch to RCU
From: David Miller @ 2009-11-02 7:56 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <4AEE6CB8.1000106@gmail.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 02 Nov 2009 06:23:04 +0100
> Use dev_get_by_index_rcu() instead of __dev_get_by_index() and
> dev_base_lock rwlock
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Man, you've been busy.... :-)
Applied, thanks!
^ permalink raw reply
* Re: [PATCH net-next-2.6] icmp: icmp_send() can avoid a dev_put()
From: David Miller @ 2009-11-02 7:57 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <4AEE6E97.5070809@gmail.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 02 Nov 2009 06:31:03 +0100
> We can avoid touching device refcount in icmp_send(),
> using dev_get_by_index_rcu()
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next-2.6] net: RCU locking for simple ioctl()
From: David Miller @ 2009-11-02 7:57 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <4AEE7131.601@gmail.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 02 Nov 2009 06:42:09 +0100
> All ioctls() implemented by dev_ifsioc_locked() :
> SIOCGIFFLAGS, SIOCGIFMETRIC, SIOCGIFMTU, SIOCGIFHWADDR,
> SIOCGIFSLAVE, SIOCGIFMAP, SIOCGIFINDEX & SIOCGIFTXQLEN
> can use RCU lock instead of dev_base_lock rwlock
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next-2.6] ifb: RCU locking avoids touching dev refcount
From: David Miller @ 2009-11-02 7:57 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <4AEE71EC.7040208@gmail.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 02 Nov 2009 06:45:16 +0100
> Avoids touching dev refcount in hotpath
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next-2.6] pppoe: RCU locking in get_item_by_addr()
From: David Miller @ 2009-11-02 7:57 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <4AEE735A.4020601@gmail.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 02 Nov 2009 06:51:22 +0100
>
> Note : this patch depends on dev_get_by_name_rcu(), not yet comitted.
Yep, I just tossed that one ine.
> [PATCH net-next-2.6] pppoe: RCU locking in get_item_by_addr()
>
> Use dev_get_by_name_rcu() instead of dev_get_by_name(),
> to avoid touching device refcount in hotpath.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
And now this one too, applied, thanks!
^ permalink raw reply
* Re: [net-next PATCH 3/4] qlge: Reduce debug print output.
From: David Miller @ 2009-11-02 8:03 UTC (permalink / raw)
To: joe; +Cc: ron.mercer, netdev
In-Reply-To: <1256942686.1917.66.camel@Joe-Laptop.home>
From: Joe Perches <joe@perches.com>
Date: Fri, 30 Oct 2009 15:44:46 -0700
> On Fri, 2009-10-30 at 15:13 -0700, Ron Mercer wrote:
>> Signed-off-by: Ron Mercer <ron.mercer@qlogic.com>
> []
>> diff --git a/drivers/net/qlge/qlge.h b/drivers/net/qlge/qlge.h
>> index b9f65e0..502c3af 100644
>> --- a/drivers/net/qlge/qlge.h
>> +++ b/drivers/net/qlge/qlge.h
>> @@ -27,6 +27,18 @@
>> dev_printk(KERN_##klevel, &((qdev)->pdev->dev), \
>> "%s: " fmt, __func__, ##args); \
>> } while (0)
>> +#if 0
>> +#define QPRINTK_DBG(qdev, nlevel, klevel, fmt, args...) \
>> + do { \
>> + if (!((qdev)->msg_enable & NETIF_MSG_##nlevel)) \
>> + ; \
>> + else \
>> + dev_printk(KERN_##klevel, &((qdev)->pdev->dev), \
>> + "%s: " fmt, __func__, ##args); \
>> + } while (0)
>> +#else
>> +#define QPRINTK_DBG(qdev, nlevel, klevel, fmt, args...)
>> +#endif
>
> This uses an inverted test and it doesn't verify the args to
> dev_printk when not #defined.
>
> How about:
I also don't like this kind of change for another reason.
The message levels are pointless if you're going to adhere to them
or not based upon some CPP define.
Either do it, or don't. Like every other driver does.
If for some reason the default is problematic, adjust the default that
you pass to netif_msg_init() or, alternatively, adjust what level the
debugging messages are assigned to.
We have all of this wonderful, full, infrastructure for message
levelling. And you can change the setting either at module load time
or via ethtool. The default is also up to you as well.
And you're going to stick a "#if 0" CPP control in there? :-/
No way, I absolutely won't accept this kind of change, there is no
need for it. There is more than enough dynamic flexibility, both at
run-time via module option and ethtool message level selections, and
at compile time via the default you can choose hoever you like.
^ permalink raw reply
* Re: [RFC, PATCH] net: suspicious test in dev_change_name()
From: David Miller @ 2009-11-02 8:05 UTC (permalink / raw)
To: jarkao2; +Cc: eric.dumazet, netdev
In-Reply-To: <4AEB7BB1.9000901@gmail.com>
From: Jarek Poplawski <jarkao2@gmail.com>
Date: Sat, 31 Oct 2009 00:50:09 +0100
> I don't think so: err stores the previous ret meaning rollback and
> is checked for this later. But somebody forgot err can store previous
> (positive) value here, so IMHO you're right: there is a bug in this
> place ;-)
Just not the one Eric is specifically fixing :-)
^ permalink raw reply
* Re: [RFC] bridge: check address size
From: David Miller @ 2009-11-02 8:07 UTC (permalink / raw)
To: shemminger; +Cc: nneul, netdev
In-Reply-To: <20091029152408.6c6cc29f@nehalam>
From: Stephen Hemminger <shemminger@vyatta.com>
Date: Thu, 29 Oct 2009 15:24:08 -0700
> - if (dev->flags & IFF_LOOPBACK || dev->type != ARPHRD_ETHER)
> + /* Don't allow bridging non ethernet like devices */
> + if (dev->flags & IFF_LOOPBACK
> + || dev->type != ARPHRD_ETHER
> + || dev->addr_len != ETH_ALEN)
Please format this as:
> + if (dev->flags & IFF_LOOPBACK ||
> + dev->type != ARPHRD_ETHER ||
> + dev->addr_len != ETH_ALEN)
What you're doing in the patch follows the GNU coding standards, no
the kernel ones. :-)
^ permalink raw reply
* Re: RFC: netdev: allow ethtool physical id to drop rtnl_lock
From: David Miller @ 2009-11-02 8:17 UTC (permalink / raw)
To: mchan; +Cc: shemminger, netdev
In-Reply-To: <1257007462.9706.9.camel@HP1>
From: "Michael Chan" <mchan@broadcom.com>
Date: Sat, 31 Oct 2009 09:44:22 -0700
>
> On Fri, 2009-10-30 at 10:42 -0700, Stephen Hemminger wrote:
>> The ethtool operation to blink LED can take an indeterminately long time,
>> blocking out other operations (via rtnl_lock). This patch is an attempt
>> to work around the problem.
>>
>> It does need more discussion, because it will mean that drivers that formerly
>> were protected from changes during blink aren't. For example, user could
>> start device blinking, and then plug in cable causing change netlink event
>> to change state or pull cable and have device come down.
>
> Yeah, the biggest concern is shutting down the device while it is still
> blinking. During shutdown, some devices are brought to low power state
> and the chip will no longer respond to I/Os to blink the LEDs. On some
> systems, this can cause bus hang or NMI.
Right, and for this reason we'll either need find some way to stop
the LED blinking when the device is brought down.
We can deal with this in a way such that we'll never need to bug
the drivers again if we want to mess with the implementation again.
Create a "netif_phys_id_loop_iter()" that, along with a netdev
pointer, takes a "u32 data" which is whatever was passed in to
ethtool_ops->id().
The drivers then structure their loops like:
while (1) {
blink_it_baby();
data = netif_phys_id_loop_iter(dev, data);
if (!data)
break;
}
Next, we take that:
if (data == 0)
data = UINT_MAX / 2;
That every driver seems to do, and stick it in the ethtool op dispatch
in net/core/ethtool.c so it doesn't need to be duplicated (and
potentially forgotten) in every implementation.
Finally, in netif_phys_id_loop_iter() we put something like:
u32 netif_phys_id_loop_iter(struct netdev *dev, u32 data)
{
if (dev->reg_state == NETREG_UNREGISTERING)
return 0;
if (msleep_interruptible(500))
return 0;
return data - 2;
}
Then, unregister somehow blocks on the ->phys_id() hitting that
NETREG_UNREGISTERING check and returning.
Anyways, you get the idea.
^ permalink raw reply
* Re: HTB accuracy on 10GbE
From: Badalian Vyacheslav @ 2009-11-02 8:17 UTC (permalink / raw)
To: Ryousei Takano; +Cc: shemminger, Linux Netdev List, takano-ryousei
In-Reply-To: <b30d1c3b0911012322v5d553fd6va56575fcf5e3daf8@mail.gmail.com>
Hello.
Also we planed convert 5-10 servers witch 1gigabit connection to one BIG server witch 10g (traffic rate in peak about 6gigabit) network card (Intel multiqueue).
Can test any patches for fix any problems in 10 gigabit connections!
Thanks!
JSC BIG Telecom
> Hi Stephen and all,
>
> I have observed a HTB accuracy problem on the Linux kernel 2.6.30 and
> the Myri-10G 10 GbE NIC.
> HTB can control the transmission rate at Gigabit speed, however it can
> not work well at 10 Gigabit speed.
>
> I asked Stephen this problem at Japan Linux Symposium. He mentioned a
> HTB bug related to the timer granularity.
> I want to know what is happen, and what should be do for fixing it.
>
> Any comments and suggestions will be welcome.
>
> For more detail, please see the following page:
> http://code.google.com/p/pspacer/wiki/HTBon10GbE
>
> Best regards,
> Ryousei
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply
* Re: kerneloops.org report for the week of October 31 2009
From: Andy Whitcroft @ 2009-11-02 8:31 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Arjan van de Ven, linux-kernel, torvalds, linux-acpi, tytso,
netdev, Eric Paris, akpm, Matt Domsch
In-Reply-To: <200911010020.20284.rjw@sisk.pl>
> This should be fixed by commit 04bf7539c08d64184736cdc5e4ad617eda77eb0f
> (PM: Make warning in suspend_test_finish() less likely to happen), at least on some
> machines that report it.
That commit has been pulled back to the first 9.10 updates kernel to
reduce the noise.
-apw
^ permalink raw reply
* Re: pull request: wireless-next-2.6 2009-10-28
From: Ingo Molnar @ 2009-11-02 9:10 UTC (permalink / raw)
To: John W. Linville
Cc: Jarek Poplawski, Johannes Berg, Bartlomiej Zolnierkiewicz,
Pekka Enberg, David Miller, linux-wireless, netdev, linux-kernel
In-Reply-To: <20091030150223.GA2586@tuxdriver.com>
* John W. Linville <linville@tuxdriver.com> wrote:
> On Fri, Oct 30, 2009 at 11:06:16AM +0000, Jarek Poplawski wrote:
>
> > There are various ways to disagree, and ignoring by John questions
> > from a merited developer both in this referenced lkml and current
> > threads looks at least strange (if not offensive) as well.
>
> Did you read the thread for which Bartlomiej provided a link earlier?
> There were ten responses (only three of them from him) in that thread.
> His comments were not ignored, they were rejected.
>
> Ever since Bartlomiej decided to tear himself away from
> drivers/staging, he has been nothing but negative -- petty, whining,
> indignat, whatever. Just what has he done to merit any special
> consideration here? Why should he have any sort of veto over rt2x00?
I got curious, as my past experience with Bartlomiej is that he is a
factual, reliable, knowledgable upstream driver developer interested in
difficult pieces of code others are reluctant to touch, for whom it is
rather atypical to get 'petty, whining, indignant'.
So i have read the thread you and Bartlomiej referenced:
http://lkml.org/lkml/2009/10/17/81
... and my understanding of that discussion is very different from
yours. Here is my annotated history of the beginnings of that
discussion:
Bartlomiej (in <200910171654.03344.bzolnier@gmail.com>) started his
review of the driver with:
| First let me say that I'm very happy to see this patch finally being
| submitted and I appreciate the effort..
|
| (I'll give it a spin on Eee 901 w/ 2.6.32-rc5 sometime later..)
Very friendly and constructive. Pretty much the Bartlomiej i have known
for years.
Then he continues with his technical observations:
| Now to the less happy part..
|
| I also used the opportunity to take a closer look at this driver and
| it seems that it needlessly adds around 2 KLOC to kernel by
| duplicating the common content of rt2800usb.h to rt2800pci.h instead
| of moving it to the shared header (like it is done in the staging
| crap drivers):
|
| [...]
|
| All in all, the total amount of the kernel code needed for
| implementing rt2800pci functionality should 1-2 KLOC instead of the
| current 5 KLOC.
Looks like a valid technical point that should be replied to in ernest.
Johannes Berg's first reply (<1255792104.3434.2.camel@johannes.local>)
ignored Bartlomiej's friendly approach and launched a combative,
emotion-laden, unconstructive (and technically inapposite) attack:
| Tell me you're kidding -- comparing 2k duplicated LOC with a driver
| that ships its own wifi stack?
Bartlomiej's reply (<1255792104.3434.2.camel@johannes.local>) ignored
the attack (gracefully) and replied to the technical portion only:
| > Tell me you're kidding -- comparing 2k duplicated LOC with a driver
| > that ships its own wifi stack?
|
| Why would I be?
|
| 1) The patch is submitted to kernel _proper_ not kernel staging so I
| see no excuse for duplicating 2-4 KLOC and it should be fixed.
|
| 2) The fact that the some staging driver consists in 90% of crap
| doesn't mean that it doesn't have some good design ideas.. (i.e.
| abstracting chipset registers access in a discussed case)
To which technical point Johannes elected not to reply. (Effectively
conceding Bartlomiej's point as per lkml discussion rules.)
[ There are similar patterns in other threads of this discussion - the
reply in (<200910181859.22413.IvDoorn@gmail.com>) and followups
were similarly dismissive (while not as combative as Johannes's reply)
- with an often offensive tone against Bartlomiej. ]
Bartlomiej followed up with his test results in another message in
<200910172318.56929.bzolnier@gmail.com>. Corroborated by Luis Correia in
<efe7343f0910180240o223ac346j3dd7c45c7460ec41@mail.gmail.com>. Both
messages were factual, constructive and friendly.
Neither failure report was replied to in that thread and remains ignored
up to today, 15 days down the line.
Alas, the portion of the story that is visible in that discussion on
lkml contradicts your claim almost 180 degrees. The person being
attacked there was Bartlomiej and i simply dont see where you got the
conclusion from that he was 'petty, whining, indignant'.
Now look at the aftermath from Bartlomiej's perspective: this
non-working driver with arguably unresponsive, unfriendly maintainers
got pulled twice (first by you and then by David), and it is now on the
unstoppable path upstream. By omission he's been forced to raise these
issues at every hop that pulls this piece of code - and it was not his
choice to be exposed to such a spiral of a workflow.
I can understand David trusting your judgement and not wanting to get
involved in the fine details, but having read the surrounding discussion
i dont understand your interpretation of the events, and i dont
understand on what basis you launched your very serious accusation, that
he is being 'petty, whining, indignant'. Every reply from him in that
thread is the exact opposite of that. Care to elaborate?
Thanks,
Ingo
^ permalink raw reply
* Re: pull request: wireless-next-2.6 2009-10-28
From: Johannes Berg @ 2009-11-02 9:23 UTC (permalink / raw)
To: Ingo Molnar
Cc: John W. Linville, Jarek Poplawski, Bartlomiej Zolnierkiewicz,
Pekka Enberg, David Miller, linux-wireless, netdev, linux-kernel
In-Reply-To: <20091102091038.GA9044@elte.hu>
[-- Attachment #1: Type: text/plain, Size: 663 bytes --]
On Mon, 2009-11-02 at 10:10 +0100, Ingo Molnar wrote:
> So i have read the thread you and Bartlomiej referenced:
>
> http://lkml.org/lkml/2009/10/17/81
>
> ... and my understanding of that discussion is very different from
> yours. Here is my annotated history of the beginnings of that
> discussion:
[snip]
You shouldn't ignore all previous interaction between Bart and us --
which wasn't pretty: http://thread.gmane.org/gmane.linux.kernel/901892
Of course we were biased when he came around with that petty code
duplication argument, since it seemed to support only his agenda of
working only with the staging drivers.
johannes
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* [PATCH net-next-2.6] packet: less dev_put() calls
From: Eric Dumazet @ 2009-11-02 9:43 UTC (permalink / raw)
To: David S. Miller; +Cc: Linux Netdev List
- packet_sendmsg_spkt() can use dev_get_by_name_rcu() to avoid touching device refcount.
- packet_getname_spkt() & packet_getname() can use dev_get_by_index_rcu() to
avoid touching device refcount too.
tpacket_snd() & packet_snd() can not use RCU yet because they can sleep when
allocating skb.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
net/packet/af_packet.c | 22 ++++++++++++----------
1 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 95ef64e..91d246d 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -437,7 +437,8 @@ static int packet_sendmsg_spkt(struct kiocb *iocb, struct socket *sock,
*/
saddr->spkt_device[13] = 0;
- dev = dev_get_by_name(sock_net(sk), saddr->spkt_device);
+ rcu_read_lock();
+ dev = dev_get_by_name_rcu(sock_net(sk), saddr->spkt_device);
err = -ENODEV;
if (dev == NULL)
goto out_unlock;
@@ -500,14 +501,13 @@ static int packet_sendmsg_spkt(struct kiocb *iocb, struct socket *sock,
*/
dev_queue_xmit(skb);
- dev_put(dev);
+ rcu_read_unlock();
return len;
out_free:
kfree_skb(skb);
out_unlock:
- if (dev)
- dev_put(dev);
+ rcu_read_unlock();
return err;
}
@@ -1518,12 +1518,13 @@ static int packet_getname_spkt(struct socket *sock, struct sockaddr *uaddr,
return -EOPNOTSUPP;
uaddr->sa_family = AF_PACKET;
- dev = dev_get_by_index(sock_net(sk), pkt_sk(sk)->ifindex);
- if (dev) {
+ rcu_read_lock();
+ dev = dev_get_by_index_rcu(sock_net(sk), pkt_sk(sk)->ifindex);
+ if (dev)
strlcpy(uaddr->sa_data, dev->name, 15);
- dev_put(dev);
- } else
+ else
memset(uaddr->sa_data, 0, 14);
+ rcu_read_unlock();
*uaddr_len = sizeof(*uaddr);
return 0;
@@ -1543,16 +1544,17 @@ static int packet_getname(struct socket *sock, struct sockaddr *uaddr,
sll->sll_family = AF_PACKET;
sll->sll_ifindex = po->ifindex;
sll->sll_protocol = po->num;
- dev = dev_get_by_index(sock_net(sk), po->ifindex);
+ rcu_read_lock();
+ dev = dev_get_by_index_rcu(sock_net(sk), po->ifindex);
if (dev) {
sll->sll_hatype = dev->type;
sll->sll_halen = dev->addr_len;
memcpy(sll->sll_addr, dev->dev_addr, dev->addr_len);
- dev_put(dev);
} else {
sll->sll_hatype = 0; /* Bad: we have no ARPHRD_UNSPEC */
sll->sll_halen = 0;
}
+ rcu_read_unlock();
*uaddr_len = offsetof(struct sockaddr_ll, sll_addr) + sll->sll_halen;
return 0;
^ permalink raw reply related
* Re: pull request: wireless-next-2.6 2009-10-28
From: Ingo Molnar @ 2009-11-02 10:07 UTC (permalink / raw)
To: Johannes Berg
Cc: John W. Linville, Jarek Poplawski, Bartlomiej Zolnierkiewicz,
Pekka Enberg, David Miller, linux-wireless, netdev, linux-kernel
In-Reply-To: <1257153807.3555.173.camel@johannes.local>
* Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2009-11-02 at 10:10 +0100, Ingo Molnar wrote:
>
> > So i have read the thread you and Bartlomiej referenced:
> >
> > http://lkml.org/lkml/2009/10/17/81
> >
> > ... and my understanding of that discussion is very different from
> > yours. Here is my annotated history of the beginnings of that
> > discussion:
>
> [snip]
>
> You shouldn't ignore all previous interaction between Bart and us --
> which wasn't pretty: http://thread.gmane.org/gmane.linux.kernel/901892
I have seen that exchange too - here's the lkml.org link for those who
like the lkml.org format:
http://lkml.org/lkml/2009/10/13/186
And i can see no supporting fact here either, for the (very serious)
accusation launched by John Linville, that Bartlomiej is 'petty,
whining, indignant'. In my reading he is the opposite of that, even in
this second thread you point out.
So, no matter how much you disagree about the code and its direction,
please either back up your assertion with specific links to a pattern of
misbehavior or apologize for the ad-hominen attacks against Bartlomiej.
> Of course we were biased when he came around with that petty code
> duplication argument, since it seemed to support only his agenda of
> working only with the staging drivers.
Why do you think that disagreeing in the past gives you the right to get
into ad-hominens? You should concentrate on the code and on the
technical side, not on the person making the argument.
Also, why do you characterise a code duplication argument as 'petty'?
Bloat and unnecessary technical forking is the #1 enemy of Linux.
Integrating code and infrastructure is the #1 strength of Linux.
Upstream subsystems/drivers running away with their private
implementations has its clear costs:
- introduces bugs
- makes drivers shallow in practice
- makes unifying drivers and infrastructure so hard down the road
- bloats the code, increases i$ footprint
I routinely refuse patches based on 'please dont duplicate' arguments,
in fact i did it once today already.
[ I dont know why drivers/staging/ is even an argument here - he argued
about the technical qualities of a new upstream driver, not about a
staging driver. Upstream drivers are to be held to higher standards,
_especially_ now that we can isolate not-clean-enough-yet drivers into
drivers/staging/, without hurting users. ]
Thanks,
Ingo
^ permalink raw reply
* [PATCH net-next-2.6] ip6tnl: less dev_put() calls
From: Eric Dumazet @ 2009-11-02 10:21 UTC (permalink / raw)
To: David S. Miller; +Cc: Linux Netdev List
Using dev_get_by_index_rcu() in ip6_tnl_rcv_ctl() & ip6_tnl_xmit_ctl()
avoids touching device refcount.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
net/ipv6/ip6_tunnel.c | 11 +++++------
1 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 6c1b5c9..1d61411 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -658,6 +658,7 @@ static void ip6ip6_dscp_ecn_decapsulate(struct ip6_tnl *t,
IP6_ECN_set_ce(ipv6_hdr(skb));
}
+/* called with rcu_read_lock() */
static inline int ip6_tnl_rcv_ctl(struct ip6_tnl *t)
{
struct ip6_tnl_parm *p = &t->parms;
@@ -668,15 +669,13 @@ static inline int ip6_tnl_rcv_ctl(struct ip6_tnl *t)
struct net_device *ldev = NULL;
if (p->link)
- ldev = dev_get_by_index(net, p->link);
+ ldev = dev_get_by_index_rcu(net, p->link);
if ((ipv6_addr_is_multicast(&p->laddr) ||
likely(ipv6_chk_addr(net, &p->laddr, ldev, 0))) &&
likely(!ipv6_chk_addr(net, &p->raddr, NULL, 0)))
ret = 1;
- if (ldev)
- dev_put(ldev);
}
return ret;
}
@@ -804,8 +803,9 @@ static inline int ip6_tnl_xmit_ctl(struct ip6_tnl *t)
if (p->flags & IP6_TNL_F_CAP_XMIT) {
struct net_device *ldev = NULL;
+ rcu_read_lock();
if (p->link)
- ldev = dev_get_by_index(net, p->link);
+ ldev = dev_get_by_index_rcu(net, p->link);
if (unlikely(!ipv6_chk_addr(net, &p->laddr, ldev, 0)))
printk(KERN_WARNING
@@ -819,8 +819,7 @@ static inline int ip6_tnl_xmit_ctl(struct ip6_tnl *t)
p->name);
else
ret = 1;
- if (ldev)
- dev_put(ldev);
+ rcu_read_unlock();
}
return ret;
}
^ permalink raw reply related
* Re: [net-next-2.6 PATCH RFC] TCPCT part 1d: generate Responder Cookie
From: William Allen Simpson @ 2009-11-02 10:39 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Linux Kernel Developers, Linux Kernel Network Developers
In-Reply-To: <4AEDCD7C.2010403@gmail.com>
Eric Dumazet wrote:
> William Allen Simpson a écrit :
>> Since October 4th, I've repeatedly asked publicly for assistance with these
>> Linux-specific memory locking constructs and cryptography. I've also sent
>> private messages. No help has been forthcoming. None. Nada.
>>
>> At this point, I've spent weeks re-spinning code that I'd understood was
>> approved a year ago. The whole project should have been finished by now!
>
> Your messages on netdev are two weeks old, not one year, and came during
> LKS. Many developpers were busy in Japan.
>
Thank you for your helpful comments. I'm not familiar with LKS. Is that a
month long project?
The message in question was "query: per cpu hash pool spinlock", sent:
"Sun, 04 Oct 2009 06:37:39 -0400".
My initial query for this series was "query: adding a sysctl", sent:
"Fri, 02 Oct 2009 00:00:05 -0400".
The earlier RFC approval was circa 1 year, 11 weeks, 2 days, 23 hours ago:
http://article.gmane.org/gmane.linux.network/102779
>> So, I'll try a larger audience. Could somebody take a look at my usage of
>> read and write locking?
>>
>> NB, I'm trying to port some 15-year-old fairly simple and straightforward
>> (single cpu) code from the KA9Q cooperative multitasking platform.
>>
>> I've examined existing code used for syncookies and TCP MD5 authenticators.
>> Neither meets my needs, as this secret is updated every few minutes. Both
>> have very different approaches. They are rarely used. My code will be
>> used on the order of tens of thousands of connections per second.
>>
>> Moreover, it seems to my naive eye that the syncookie per cpu code simply
>> doesn't work properly. The workspace is allocated per cpu, but the cpu
>> could change during the extensive SHA1 computations. Bug?
>>
The (buggy?) syncookie code that I'm avoiding/replacing is:
static DEFINE_PER_CPU(__u32 [16 + 5 + SHA_WORKSPACE_WORDS],
ipv4_cookie_scratch);
static u32 cookie_hash(__be32 saddr, __be32 daddr, __be16 sport, __be16 dport,
u32 count, int c)
{
__u32 *tmp = __get_cpu_var(ipv4_cookie_scratch);
memcpy(tmp + 4, syncookie_secret[c], sizeof(syncookie_secret[c]));
tmp[0] = (__force u32)saddr;
tmp[1] = (__force u32)daddr;
tmp[2] = ((__force u32)sport << 16) + (__force u32)dport;
tmp[3] = count;
sha_transform(tmp + 16, (__u8 *)tmp, tmp + 16 + 5);
return tmp[17];
}
It appears to me that the operations could be interrupted at any time, and
the fairly expensive sha transform (or probably any of the assignments)
could be corrupted?
That is, cpu0 grabs a scratch area, copies some data into it, interrupts,
cpu0 comes along again with another packet, points into the same workspace,
mashes the data, while cpu1 completes the previous operation with the
old tmp pointer on the stack.
Worse, this is called twice, each time getting tmp, and mashing the data,
and at the same time others are calling it twice more for verification.
Since syncookies only occur under stress, the code isn't well tested, and
the only symptom would be the returned packet would be dropped after
failing to verify. Since this only happens when lots of packets are
arriving, dropped packets probably aren't noticed.
However, that would be unacceptable for my code.
> This patch looks fine, but I dont see how this new function is used.
>
It is used at the places in the previous numbered patch (part 1c) that
currently say, "secret recipe not yet implemented".
And perhaps to replace the syncookie code mentioned above.
> Some points :
>
> 1) We are working hard to remove rwlocks from network stack, so please dont
> add a new one.
Botheration! I wish that was *documented* somewhere, perhaps in:
Documentation/spinlocks.txt
> add a new one. You probably can use a seqlock or RCU, or a server handling
> 10.000 connections request per second on many NIC will hit this rwlock.
>
Yep. Nobody in the tcp code is using them, but RCU looks interesting, and
may be well suited. I'll read up on it.
> 2)
>
> } else if (unlikely(time_after(jiffy, tcp_secret_primary->expires))) {
> get_random_bytes(secrets, sizeof(secrets));
>
> write_lock(&tcp_secret_locker);
>
> It would be better to first get the lock, then get random_bytes, in order
> not wasting entropy.
>
I'd put that outside the locked section as it's considerably more expensive
(hashing) than a memcpy. Entropy is never "wasted", as good cryptographic
random functions never reveal underlying entropy. I know there were some
papers a few years ago that found flaws in Linux random functions, but
surely that's been fixed. I assume you folks are using Yarrow or its
successors by now.
In any case, perhaps this RCU thingy obviates that issue.
>
> 3) If you change secret ever 600 seconds, it might be better to use a timer
> so that you dont have to check expiration and this logic at each SYN packet.
> (Disociate the lookup (read-only, done many time per second) from the updates
> (trigerred by a timer every 600 secs))
>
Part of the logic is not to update the generation secret until a new packet
arrives, allowing maximal entropy to be gathered beforehand. This is
especially worrisome for the first packets after booting. It takes into
account inter-packet arrival timing, and expiring after MSL. So, a timer
isn't feasible in this instance.
> (Not counting you'll probably need to use a similar lookup algo for the ACK
> packet coming from client)
>
Yes. A timer could be used to expire and clear old verification secrets,
and I'll look into that in the future ACK patch.
Thanks again.
^ permalink raw reply
* Re: [net-next-2.6 PATCH v4 2/3] TCPCT part 1b: sysctl_tcp_cookie_size, socket option TCP_COOKIE_TRANSACTIONS, functions
From: William Allen Simpson @ 2009-11-02 10:45 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Linux Kernel Network Developers
In-Reply-To: <4AEDDDF3.3070102@gmail.com>
Eric Dumazet wrote:
> William Allen Simpson a écrit :
>> These functions will also be used in subsequent patches that implement
>> additional features.
>>
>
> +#define TCP_EXTEND_TIMESTAMP (1 << 4) /* Initiate 64-bit timestamps */
>
> What is this for ?
>
Reserving a bit in the sockopt for a future feature (described in the
aforementioned end2end-interest email messages).
Certainly, reserving bits seems to be common elsewhere in Linux code. I'm
trying to coordinate this with the *BSD developers, too, so it helps to keep
API documentation synchronized. Is that a problem?
^ permalink raw reply
* Re: [net-next-2.6 PATCH RFC] TCPCT part 1d: generate Responder Cookie
From: David Miller @ 2009-11-02 10:50 UTC (permalink / raw)
To: william.allen.simpson; +Cc: eric.dumazet, linux-kernel, netdev
In-Reply-To: <4AEEB6D2.7090505@gmail.com>
From: William Allen Simpson <william.allen.simpson@gmail.com>
Date: Mon, 02 Nov 2009 05:39:14 -0500
> Thank you for your helpful comments. I'm not familiar with LKS. Is
> that a month long project?
It's the Linux Kernel Summit. It's well publicized:
http://events.linuxfoundation.org/events/kernel-summit
It's the yearly meeting of the top Linux kernel developers, it usually
lasts several days and since a large portion of the kernel developers
who attend have to travel on 10+ hour flights to reach the summit
location most choose to pad their trip on either side by several days
in order to recover from jet lag and other effects of travelling.
This year most kernel developers stayed for the rest of the week in
order to attend the Japan Linux Symposium.
Many people were offline a total of 2 weeks or more because of these
events. Some still haven't returned from Japan.
But that's neither here nor there, people sometimes are simply
overloaded with work that is important to them, or from their
view more important than reviewing your work.
Sometimes that happens and you just have to be patient and
understanding.
Complaining that your work isn't getting looked at in a timely manner
will always have the exact opposite effect that you want, it makes
people have a smaller desire to look at your stuff.
^ permalink raw reply
* Re: [net-next-2.6 PATCH v4 2/3] TCPCT part 1b: sysctl_tcp_cookie_size, socket option TCP_COOKIE_TRANSACTIONS, functions
From: David Miller @ 2009-11-02 10:51 UTC (permalink / raw)
To: william.allen.simpson; +Cc: eric.dumazet, netdev
In-Reply-To: <4AEEB842.1070705@gmail.com>
From: William Allen Simpson <william.allen.simpson@gmail.com>
Date: Mon, 02 Nov 2009 05:45:22 -0500
> Is that a problem?
Yes, it is a problem.
You can allocate the bit when you add the feature.
^ permalink raw reply
* Re: [net-next-2.6 PATCH RFC] TCPCT part 1d: generate Responder Cookie
From: Eric Dumazet @ 2009-11-02 10:56 UTC (permalink / raw)
To: William Allen Simpson
Cc: Linux Kernel Developers, Linux Kernel Network Developers
In-Reply-To: <4AEEB6D2.7090505@gmail.com>
William Allen Simpson a écrit :
>
> The (buggy?) syncookie code that I'm avoiding/replacing is:
>
> static DEFINE_PER_CPU(__u32 [16 + 5 + SHA_WORKSPACE_WORDS],
> ipv4_cookie_scratch);
>
> static u32 cookie_hash(__be32 saddr, __be32 daddr, __be16 sport, __be16
> dport,
> u32 count, int c)
> {
> __u32 *tmp = __get_cpu_var(ipv4_cookie_scratch);
>
> memcpy(tmp + 4, syncookie_secret[c], sizeof(syncookie_secret[c]));
> tmp[0] = (__force u32)saddr;
> tmp[1] = (__force u32)daddr;
> tmp[2] = ((__force u32)sport << 16) + (__force u32)dport;
> tmp[3] = count;
> sha_transform(tmp + 16, (__u8 *)tmp, tmp + 16 + 5);
>
> return tmp[17];
> }
>
> It appears to me that the operations could be interrupted at any time, and
> the fairly expensive sha transform (or probably any of the assignments)
> could be corrupted?
>
> That is, cpu0 grabs a scratch area, copies some data into it, interrupts,
> cpu0 comes along again with another packet, points into the same workspace,
> mashes the data, while cpu1 completes the previous operation with the
> old tmp pointer on the stack.
>
> Worse, this is called twice, each time getting tmp, and mashing the data,
> and at the same time others are calling it twice more for verification.
>
> Since syncookies only occur under stress, the code isn't well tested, and
> the only symptom would be the returned packet would be dropped after
> failing to verify. Since this only happens when lots of packets are
> arriving, dropped packets probably aren't noticed.
>
> However, that would be unacceptable for my code.
>
cookie_hash() runs in a non preemptable context. CPU cannot change under us.
(or else, we would not use __get_cpu_var(ipv4_cookie_scratch); )
And of course, each cpu gets its own scratch area, thanks to __get_cpu_var()
^ permalink raw reply
* [PATCH net-next-2.6] ipv6: no more dev_put() in inet6_bind()
From: Eric Dumazet @ 2009-11-02 11:10 UTC (permalink / raw)
To: David S. Miller; +Cc: Linux Netdev List
Avoids touching device refcount in inet6_bind(), thanks to RCU
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
net/ipv6/af_inet6.c | 17 +++++++++--------
1 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index b6d0588..9105b25 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -314,6 +314,7 @@ int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
if (addr_type != IPV6_ADDR_ANY) {
struct net_device *dev = NULL;
+ rcu_read_lock();
if (addr_type & IPV6_ADDR_LINKLOCAL) {
if (addr_len >= sizeof(struct sockaddr_in6) &&
addr->sin6_scope_id) {
@@ -326,12 +327,12 @@ int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
/* Binding to link-local address requires an interface */
if (!sk->sk_bound_dev_if) {
err = -EINVAL;
- goto out;
+ goto out_unlock;
}
- dev = dev_get_by_index(net, sk->sk_bound_dev_if);
+ dev = dev_get_by_index_rcu(net, sk->sk_bound_dev_if);
if (!dev) {
err = -ENODEV;
- goto out;
+ goto out_unlock;
}
}
@@ -342,14 +343,11 @@ int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
if (!(addr_type & IPV6_ADDR_MULTICAST)) {
if (!ipv6_chk_addr(net, &addr->sin6_addr,
dev, 0)) {
- if (dev)
- dev_put(dev);
err = -EADDRNOTAVAIL;
- goto out;
+ goto out_unlock;
}
}
- if (dev)
- dev_put(dev);
+ rcu_read_unlock();
}
}
@@ -381,6 +379,9 @@ int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
out:
release_sock(sk);
return err;
+out_unlock:
+ rcu_read_unlock();
+ goto out;
}
EXPORT_SYMBOL(inet6_bind);
^ permalink raw reply related
* RE: Patch: Fix userland build failure in linux/if_tunnel.h
From: Sledz, Steffen @ 2009-11-02 11:15 UTC (permalink / raw)
To: Sergei Poselenov; +Cc: wd, linux-kernel, netdev
In-Reply-To: <20091102135750.436a7b54@emcraft.com>
Hello Sergei, hello other maintainers,
> > i'm working for the OpenEmbedded project and we hit the same busybox
> > problem you mentioned in <http://patchwork.kernel.org/patch/31164/>.
> >
> > It doesn't look like you got much feedback on you patch suggestion.
> > Did it made it's way to the kernel sources? Or where there any
> > objections on it?
> Hm, I received a confirmation, that my fix was applied
> http://www.spinics.net/lists/netdev/msg103860.html
No, i think it didn't. This confirmation is related to a patch from
Joe Perches to the MAINTAINERS file.
> However, it indeed didn't find their way into the mainline.
> I have no idea why, received no objections.
What are the maintainers saying?
Steffen
^ permalink raw reply
* Re: Patch: Fix userland build failure in linux/if_tunnel.h
From: Sergei Poselenov @ 2009-11-02 10:57 UTC (permalink / raw)
To: Steffen Sledz, Steffen Sledz; +Cc: sledz, wd, linux-kernel, netdev
In-Reply-To: <4AEDCBA6.7040206@dresearch.de>
Hello Steffen,
Hm, I received a confirmation, that my fix was applied
http://www.spinics.net/lists/netdev/msg103860.html
However, it indeed didn't find their way into the mainline.
I have no idea why, received no objections.
I'm sending the reply to the resp. mailing lists too; hope you don't
mind.
Regards,
Sergei
On Sun, 01 Nov 2009 18:55:50 +0100
Steffen Sledz <sledz@dresearch.de> wrote:
> Hello Sergei,
>
> i'm working for the OpenEmbedded project and we hit the same busybox
> problem you mentioned in <http://patchwork.kernel.org/patch/31164/>.
>
> It doesn't look like you got much feedback on you patch suggestion.
> Did it made it's way to the kernel sources? Or where there any
> objections on it?
>
> Steffen
^ permalink raw reply
* [PATCH net-next-2.6] ipv6: no more dev_put() in datagram_send_ctl()
From: Eric Dumazet @ 2009-11-02 11:21 UTC (permalink / raw)
To: David S. Miller; +Cc: Linux Netdev List
Avoids touching device refcount in datagram_send_ctl(), thanks to RCU
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
net/ipv6/datagram.c | 14 +++++++++-----
1 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index 9f70452..e6f9cdf 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -537,12 +537,17 @@ int datagram_send_ctl(struct net *net,
addr_type = __ipv6_addr_type(&src_info->ipi6_addr);
+ rcu_read_lock();
if (fl->oif) {
- dev = dev_get_by_index(net, fl->oif);
- if (!dev)
+ dev = dev_get_by_index_rcu(net, fl->oif);
+ if (!dev) {
+ rcu_read_unlock();
return -ENODEV;
- } else if (addr_type & IPV6_ADDR_LINKLOCAL)
+ }
+ } else if (addr_type & IPV6_ADDR_LINKLOCAL) {
+ rcu_read_unlock();
return -EINVAL;
+ }
if (addr_type != IPV6_ADDR_ANY) {
int strict = __ipv6_addr_src_scope(addr_type) <= IPV6_ADDR_SCOPE_LINKLOCAL;
@@ -553,8 +558,7 @@ int datagram_send_ctl(struct net *net,
ipv6_addr_copy(&fl->fl6_src, &src_info->ipi6_addr);
}
- if (dev)
- dev_put(dev);
+ rcu_read_unlock();
if (err)
goto exit_f;
^ permalink raw reply related
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