* Re: [PATCH net-next] Phonet: use rwlock for sockets list
From: Eric Dumazet @ 2009-11-06 13:44 UTC (permalink / raw)
To: Rémi Denis-Courmont; +Cc: netdev, Rémi Denis-Courmont
In-Reply-To: <1257514891-18917-1-git-send-email-remi@remlab.net>
Rémi Denis-Courmont a écrit :
> From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
>
> Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> ---
> net/phonet/socket.c | 24 ++++++++++++------------
> 1 files changed, 12 insertions(+), 12 deletions(-)
Hmm... rwlocks are bad...
Would you care to explain why you introduce a rwlock ?
Thanks
^ permalink raw reply
* Re: [PATCH net-next] Phonet: use rwlock for sockets list
From: Rémi Denis-Courmont @ 2009-11-06 13:54 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, Rémi Denis-Courmont
In-Reply-To: <4AF42834.7010707@gmail.com>
On Fri, 06 Nov 2009 14:44:20 +0100, Eric Dumazet <eric.dumazet@gmail.com>
wrote:
> Rémi Denis-Courmont a écrit :
>> From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
>>
>> Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
>> ---
>> net/phonet/socket.c | 24 ++++++++++++------------
>> 1 files changed, 12 insertions(+), 12 deletions(-)
>
> Hmm... rwlocks are bad...
They're in many places throughout the non-IP families...
> Would you care to explain why you introduce a rwlock ?
It seems better than a spinlock, assuming that sockets are
created/destroyed more seldom than they receive packets. And then
sk_for_each_rcu does not exist. I am sure there is a good reason for that,
though I wouldn't know. I guess I should try to use RCU hlist_nulls then?
--
Rémi Denis-Courmont
^ permalink raw reply
* [PATCH, WTF] atm: move all compat_ioctl handling to atm/ioctl.c
From: Arnd Bergmann @ 2009-11-06 14:00 UTC (permalink / raw)
To: David Woodhouse; +Cc: David Miller, LKML, netdev, Eric Dumazet
For some interesting historic reasons that are not at all clear to me,
we seem to have two implementations of the ATM ioctl compatibility
layer in the kernel, and we use a combination of the two.
For ioctl numbers that have the same identifier on 32 and 64 bit systems,
we go directly through the compat_ioctl socket operation, for those that
differ, we do a conversion in fs/compat_ioctl.c.
This patch moves both variants into the vcc_compat_ioctl() function, while
preserving the current behaviour. It also kills off the COMPATIBLE_IOCTL
definitions that we never use here.
Doing it this way is clearly not a good solution, but I hope it is a
step into the right direction, so that someone is able to clean up this
mess for real.
Found while trying to untangle the compat socket ioctl mess
in fs/compat_ioctl.c.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
fs/compat_ioctl.c | 205 -----------------------------------------------------
net/atm/ioctl.c | 172 ++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 171 insertions(+), 206 deletions(-)
diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index f91fd51..8b78b5f 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -1212,170 +1212,6 @@ static int do_smb_getmountuid(unsigned int fd, unsigned int cmd, unsigned long a
return err;
}
-struct atmif_sioc32 {
- compat_int_t number;
- compat_int_t length;
- compat_caddr_t arg;
-};
-
-struct atm_iobuf32 {
- compat_int_t length;
- compat_caddr_t buffer;
-};
-
-#define ATM_GETLINKRATE32 _IOW('a', ATMIOC_ITF+1, struct atmif_sioc32)
-#define ATM_GETNAMES32 _IOW('a', ATMIOC_ITF+3, struct atm_iobuf32)
-#define ATM_GETTYPE32 _IOW('a', ATMIOC_ITF+4, struct atmif_sioc32)
-#define ATM_GETESI32 _IOW('a', ATMIOC_ITF+5, struct atmif_sioc32)
-#define ATM_GETADDR32 _IOW('a', ATMIOC_ITF+6, struct atmif_sioc32)
-#define ATM_RSTADDR32 _IOW('a', ATMIOC_ITF+7, struct atmif_sioc32)
-#define ATM_ADDADDR32 _IOW('a', ATMIOC_ITF+8, struct atmif_sioc32)
-#define ATM_DELADDR32 _IOW('a', ATMIOC_ITF+9, struct atmif_sioc32)
-#define ATM_GETCIRANGE32 _IOW('a', ATMIOC_ITF+10, struct atmif_sioc32)
-#define ATM_SETCIRANGE32 _IOW('a', ATMIOC_ITF+11, struct atmif_sioc32)
-#define ATM_SETESI32 _IOW('a', ATMIOC_ITF+12, struct atmif_sioc32)
-#define ATM_SETESIF32 _IOW('a', ATMIOC_ITF+13, struct atmif_sioc32)
-#define ATM_GETSTAT32 _IOW('a', ATMIOC_SARCOM+0, struct atmif_sioc32)
-#define ATM_GETSTATZ32 _IOW('a', ATMIOC_SARCOM+1, struct atmif_sioc32)
-#define ATM_GETLOOP32 _IOW('a', ATMIOC_SARCOM+2, struct atmif_sioc32)
-#define ATM_SETLOOP32 _IOW('a', ATMIOC_SARCOM+3, struct atmif_sioc32)
-#define ATM_QUERYLOOP32 _IOW('a', ATMIOC_SARCOM+4, struct atmif_sioc32)
-
-static struct {
- unsigned int cmd32;
- unsigned int cmd;
-} atm_ioctl_map[] = {
- { ATM_GETLINKRATE32, ATM_GETLINKRATE },
- { ATM_GETNAMES32, ATM_GETNAMES },
- { ATM_GETTYPE32, ATM_GETTYPE },
- { ATM_GETESI32, ATM_GETESI },
- { ATM_GETADDR32, ATM_GETADDR },
- { ATM_RSTADDR32, ATM_RSTADDR },
- { ATM_ADDADDR32, ATM_ADDADDR },
- { ATM_DELADDR32, ATM_DELADDR },
- { ATM_GETCIRANGE32, ATM_GETCIRANGE },
- { ATM_SETCIRANGE32, ATM_SETCIRANGE },
- { ATM_SETESI32, ATM_SETESI },
- { ATM_SETESIF32, ATM_SETESIF },
- { ATM_GETSTAT32, ATM_GETSTAT },
- { ATM_GETSTATZ32, ATM_GETSTATZ },
- { ATM_GETLOOP32, ATM_GETLOOP },
- { ATM_SETLOOP32, ATM_SETLOOP },
- { ATM_QUERYLOOP32, ATM_QUERYLOOP }
-};
-
-#define NR_ATM_IOCTL ARRAY_SIZE(atm_ioctl_map)
-
-static int do_atm_iobuf(unsigned int fd, unsigned int cmd, unsigned long arg)
-{
- struct atm_iobuf __user *iobuf;
- struct atm_iobuf32 __user *iobuf32;
- u32 data;
- void __user *datap;
- int len, err;
-
- iobuf = compat_alloc_user_space(sizeof(*iobuf));
- iobuf32 = compat_ptr(arg);
-
- if (get_user(len, &iobuf32->length) ||
- get_user(data, &iobuf32->buffer))
- return -EFAULT;
- datap = compat_ptr(data);
- if (put_user(len, &iobuf->length) ||
- put_user(datap, &iobuf->buffer))
- return -EFAULT;
-
- err = sys_ioctl(fd, cmd, (unsigned long)iobuf);
-
- if (!err) {
- if (copy_in_user(&iobuf32->length, &iobuf->length,
- sizeof(int)))
- err = -EFAULT;
- }
-
- return err;
-}
-
-static int do_atmif_sioc(unsigned int fd, unsigned int cmd, unsigned long arg)
-{
- struct atmif_sioc __user *sioc;
- struct atmif_sioc32 __user *sioc32;
- u32 data;
- void __user *datap;
- int err;
-
- sioc = compat_alloc_user_space(sizeof(*sioc));
- sioc32 = compat_ptr(arg);
-
- if (copy_in_user(&sioc->number, &sioc32->number, 2 * sizeof(int)) ||
- get_user(data, &sioc32->arg))
- return -EFAULT;
- datap = compat_ptr(data);
- if (put_user(datap, &sioc->arg))
- return -EFAULT;
-
- err = sys_ioctl(fd, cmd, (unsigned long) sioc);
-
- if (!err) {
- if (copy_in_user(&sioc32->length, &sioc->length,
- sizeof(int)))
- err = -EFAULT;
- }
- return err;
-}
-
-static int do_atm_ioctl(unsigned int fd, unsigned int cmd32, unsigned long arg)
-{
- int i;
- unsigned int cmd = 0;
-
- switch (cmd32) {
- case SONET_GETSTAT:
- case SONET_GETSTATZ:
- case SONET_GETDIAG:
- case SONET_SETDIAG:
- case SONET_CLRDIAG:
- case SONET_SETFRAMING:
- case SONET_GETFRAMING:
- case SONET_GETFRSENSE:
- return do_atmif_sioc(fd, cmd32, arg);
- }
-
- for (i = 0; i < NR_ATM_IOCTL; i++) {
- if (cmd32 == atm_ioctl_map[i].cmd32) {
- cmd = atm_ioctl_map[i].cmd;
- break;
- }
- }
- if (i == NR_ATM_IOCTL)
- return -EINVAL;
-
- switch (cmd) {
- case ATM_GETNAMES:
- return do_atm_iobuf(fd, cmd, arg);
-
- case ATM_GETLINKRATE:
- case ATM_GETTYPE:
- case ATM_GETESI:
- case ATM_GETADDR:
- case ATM_RSTADDR:
- case ATM_ADDADDR:
- case ATM_DELADDR:
- case ATM_GETCIRANGE:
- case ATM_SETCIRANGE:
- case ATM_SETESI:
- case ATM_SETESIF:
- case ATM_GETSTAT:
- case ATM_GETSTATZ:
- case ATM_GETLOOP:
- case ATM_SETLOOP:
- case ATM_QUERYLOOP:
- return do_atmif_sioc(fd, cmd, arg);
- }
-
- return -EINVAL;
-}
-
static __used int
ret_einval(unsigned int fd, unsigned int cmd, unsigned long arg)
{
@@ -2311,22 +2147,6 @@ COMPATIBLE_IOCTL(RAW_SETBIND)
COMPATIBLE_IOCTL(RAW_GETBIND)
/* SMB ioctls which do not need any translations */
COMPATIBLE_IOCTL(SMB_IOC_NEWCONN)
-/* Little a */
-COMPATIBLE_IOCTL(ATMSIGD_CTRL)
-COMPATIBLE_IOCTL(ATMARPD_CTRL)
-COMPATIBLE_IOCTL(ATMLEC_CTRL)
-COMPATIBLE_IOCTL(ATMLEC_MCAST)
-COMPATIBLE_IOCTL(ATMLEC_DATA)
-COMPATIBLE_IOCTL(ATM_SETSC)
-COMPATIBLE_IOCTL(SIOCSIFATMTCP)
-COMPATIBLE_IOCTL(SIOCMKCLIP)
-COMPATIBLE_IOCTL(ATMARP_MKIP)
-COMPATIBLE_IOCTL(ATMARP_SETENTRY)
-COMPATIBLE_IOCTL(ATMARP_ENCAP)
-COMPATIBLE_IOCTL(ATMTCP_CREATE)
-COMPATIBLE_IOCTL(ATMTCP_REMOVE)
-COMPATIBLE_IOCTL(ATMMPC_CTRL)
-COMPATIBLE_IOCTL(ATMMPC_DATA)
/* Watchdog */
COMPATIBLE_IOCTL(WDIOC_GETSUPPORT)
COMPATIBLE_IOCTL(WDIOC_GETSTATUS)
@@ -2613,31 +2433,6 @@ HANDLE_IOCTL(KDFONTOP, do_kdfontop_ioctl)
/* One SMB ioctl needs translations. */
#define SMB_IOC_GETMOUNTUID_32 _IOR('u', 1, compat_uid_t)
HANDLE_IOCTL(SMB_IOC_GETMOUNTUID_32, do_smb_getmountuid)
-HANDLE_IOCTL(ATM_GETLINKRATE32, do_atm_ioctl)
-HANDLE_IOCTL(ATM_GETNAMES32, do_atm_ioctl)
-HANDLE_IOCTL(ATM_GETTYPE32, do_atm_ioctl)
-HANDLE_IOCTL(ATM_GETESI32, do_atm_ioctl)
-HANDLE_IOCTL(ATM_GETADDR32, do_atm_ioctl)
-HANDLE_IOCTL(ATM_RSTADDR32, do_atm_ioctl)
-HANDLE_IOCTL(ATM_ADDADDR32, do_atm_ioctl)
-HANDLE_IOCTL(ATM_DELADDR32, do_atm_ioctl)
-HANDLE_IOCTL(ATM_GETCIRANGE32, do_atm_ioctl)
-HANDLE_IOCTL(ATM_SETCIRANGE32, do_atm_ioctl)
-HANDLE_IOCTL(ATM_SETESI32, do_atm_ioctl)
-HANDLE_IOCTL(ATM_SETESIF32, do_atm_ioctl)
-HANDLE_IOCTL(ATM_GETSTAT32, do_atm_ioctl)
-HANDLE_IOCTL(ATM_GETSTATZ32, do_atm_ioctl)
-HANDLE_IOCTL(ATM_GETLOOP32, do_atm_ioctl)
-HANDLE_IOCTL(ATM_SETLOOP32, do_atm_ioctl)
-HANDLE_IOCTL(ATM_QUERYLOOP32, do_atm_ioctl)
-HANDLE_IOCTL(SONET_GETSTAT, do_atm_ioctl)
-HANDLE_IOCTL(SONET_GETSTATZ, do_atm_ioctl)
-HANDLE_IOCTL(SONET_GETDIAG, do_atm_ioctl)
-HANDLE_IOCTL(SONET_SETDIAG, do_atm_ioctl)
-HANDLE_IOCTL(SONET_CLRDIAG, do_atm_ioctl)
-HANDLE_IOCTL(SONET_SETFRAMING, do_atm_ioctl)
-HANDLE_IOCTL(SONET_GETFRAMING, do_atm_ioctl)
-HANDLE_IOCTL(SONET_GETFRSENSE, do_atm_ioctl)
/* block stuff */
#ifdef CONFIG_BLOCK
/* loop */
diff --git a/net/atm/ioctl.c b/net/atm/ioctl.c
index 4da8892..163547d 100644
--- a/net/atm/ioctl.c
+++ b/net/atm/ioctl.c
@@ -191,8 +191,178 @@ int vcc_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
}
#ifdef CONFIG_COMPAT
+struct atmif_sioc32 {
+ compat_int_t number;
+ compat_int_t length;
+ compat_caddr_t arg;
+};
+
+struct atm_iobuf32 {
+ compat_int_t length;
+ compat_caddr_t buffer;
+};
+
+#define ATM_GETLINKRATE32 _IOW('a', ATMIOC_ITF+1, struct atmif_sioc32)
+#define ATM_GETNAMES32 _IOW('a', ATMIOC_ITF+3, struct atm_iobuf32)
+#define ATM_GETTYPE32 _IOW('a', ATMIOC_ITF+4, struct atmif_sioc32)
+#define ATM_GETESI32 _IOW('a', ATMIOC_ITF+5, struct atmif_sioc32)
+#define ATM_GETADDR32 _IOW('a', ATMIOC_ITF+6, struct atmif_sioc32)
+#define ATM_RSTADDR32 _IOW('a', ATMIOC_ITF+7, struct atmif_sioc32)
+#define ATM_ADDADDR32 _IOW('a', ATMIOC_ITF+8, struct atmif_sioc32)
+#define ATM_DELADDR32 _IOW('a', ATMIOC_ITF+9, struct atmif_sioc32)
+#define ATM_GETCIRANGE32 _IOW('a', ATMIOC_ITF+10, struct atmif_sioc32)
+#define ATM_SETCIRANGE32 _IOW('a', ATMIOC_ITF+11, struct atmif_sioc32)
+#define ATM_SETESI32 _IOW('a', ATMIOC_ITF+12, struct atmif_sioc32)
+#define ATM_SETESIF32 _IOW('a', ATMIOC_ITF+13, struct atmif_sioc32)
+#define ATM_GETSTAT32 _IOW('a', ATMIOC_SARCOM+0, struct atmif_sioc32)
+#define ATM_GETSTATZ32 _IOW('a', ATMIOC_SARCOM+1, struct atmif_sioc32)
+#define ATM_GETLOOP32 _IOW('a', ATMIOC_SARCOM+2, struct atmif_sioc32)
+#define ATM_SETLOOP32 _IOW('a', ATMIOC_SARCOM+3, struct atmif_sioc32)
+#define ATM_QUERYLOOP32 _IOW('a', ATMIOC_SARCOM+4, struct atmif_sioc32)
+
+static struct {
+ unsigned int cmd32;
+ unsigned int cmd;
+} atm_ioctl_map[] = {
+ { ATM_GETLINKRATE32, ATM_GETLINKRATE },
+ { ATM_GETNAMES32, ATM_GETNAMES },
+ { ATM_GETTYPE32, ATM_GETTYPE },
+ { ATM_GETESI32, ATM_GETESI },
+ { ATM_GETADDR32, ATM_GETADDR },
+ { ATM_RSTADDR32, ATM_RSTADDR },
+ { ATM_ADDADDR32, ATM_ADDADDR },
+ { ATM_DELADDR32, ATM_DELADDR },
+ { ATM_GETCIRANGE32, ATM_GETCIRANGE },
+ { ATM_SETCIRANGE32, ATM_SETCIRANGE },
+ { ATM_SETESI32, ATM_SETESI },
+ { ATM_SETESIF32, ATM_SETESIF },
+ { ATM_GETSTAT32, ATM_GETSTAT },
+ { ATM_GETSTATZ32, ATM_GETSTATZ },
+ { ATM_GETLOOP32, ATM_GETLOOP },
+ { ATM_SETLOOP32, ATM_SETLOOP },
+ { ATM_QUERYLOOP32, ATM_QUERYLOOP }
+};
+
+#define NR_ATM_IOCTL ARRAY_SIZE(atm_ioctl_map)
+
+static int do_atm_iobuf(struct socket *sock, unsigned int cmd, unsigned long arg)
+{
+ struct atm_iobuf __user *iobuf;
+ struct atm_iobuf32 __user *iobuf32;
+ u32 data;
+ void __user *datap;
+ int len, err;
+
+ iobuf = compat_alloc_user_space(sizeof(*iobuf));
+ iobuf32 = compat_ptr(arg);
+
+ if (get_user(len, &iobuf32->length) ||
+ get_user(data, &iobuf32->buffer))
+ return -EFAULT;
+ datap = compat_ptr(data);
+ if (put_user(len, &iobuf->length) ||
+ put_user(datap, &iobuf->buffer))
+ return -EFAULT;
+
+ err = do_vcc_ioctl(sock, cmd, (unsigned long)iobuf, 0);
+
+ if (!err) {
+ if (copy_in_user(&iobuf32->length, &iobuf->length,
+ sizeof(int)))
+ err = -EFAULT;
+ }
+
+ return err;
+}
+
+static int do_atmif_sioc(struct socket *sock, unsigned int cmd, unsigned long arg)
+{
+ struct atmif_sioc __user *sioc;
+ struct atmif_sioc32 __user *sioc32;
+ u32 data;
+ void __user *datap;
+ int err;
+
+ sioc = compat_alloc_user_space(sizeof(*sioc));
+ sioc32 = compat_ptr(arg);
+
+ if (copy_in_user(&sioc->number, &sioc32->number, 2 * sizeof(int)) ||
+ get_user(data, &sioc32->arg))
+ return -EFAULT;
+ datap = compat_ptr(data);
+ if (put_user(datap, &sioc->arg))
+ return -EFAULT;
+
+ err = do_vcc_ioctl(sock, cmd, (unsigned long) sioc, 0);
+
+ if (!err) {
+ if (copy_in_user(&sioc32->length, &sioc->length,
+ sizeof(int)))
+ err = -EFAULT;
+ }
+ return err;
+}
+
+static int do_atm_ioctl(struct socket *sock, unsigned int cmd32, unsigned long arg)
+{
+ int i;
+ unsigned int cmd = 0;
+
+ switch (cmd32) {
+ case SONET_GETSTAT:
+ case SONET_GETSTATZ:
+ case SONET_GETDIAG:
+ case SONET_SETDIAG:
+ case SONET_CLRDIAG:
+ case SONET_SETFRAMING:
+ case SONET_GETFRAMING:
+ case SONET_GETFRSENSE:
+ return do_atmif_sioc(sock, cmd32, arg);
+ }
+
+ for (i = 0; i < NR_ATM_IOCTL; i++) {
+ if (cmd32 == atm_ioctl_map[i].cmd32) {
+ cmd = atm_ioctl_map[i].cmd;
+ break;
+ }
+ }
+ if (i == NR_ATM_IOCTL)
+ return -EINVAL;
+
+ switch (cmd) {
+ case ATM_GETNAMES:
+ return do_atm_iobuf(sock, cmd, arg);
+
+ case ATM_GETLINKRATE:
+ case ATM_GETTYPE:
+ case ATM_GETESI:
+ case ATM_GETADDR:
+ case ATM_RSTADDR:
+ case ATM_ADDADDR:
+ case ATM_DELADDR:
+ case ATM_GETCIRANGE:
+ case ATM_SETCIRANGE:
+ case ATM_SETESI:
+ case ATM_SETESIF:
+ case ATM_GETSTAT:
+ case ATM_GETSTATZ:
+ case ATM_GETLOOP:
+ case ATM_SETLOOP:
+ case ATM_QUERYLOOP:
+ return do_atmif_sioc(sock, cmd, arg);
+ }
+
+ return -EINVAL;
+}
+
int vcc_compat_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
{
- return do_vcc_ioctl(sock, cmd, arg, 1);
+ int ret;
+
+ ret = do_vcc_ioctl(sock, cmd, arg, 1);
+ if (ret != -ENOIOCTLCMD)
+ return ret;
+
+ return do_atm_ioctl(sock, cmd, arg);
}
#endif
^ permalink raw reply related
* Re: [PATCH net-next] Phonet: use rwlock for sockets list
From: Eric Dumazet @ 2009-11-06 14:00 UTC (permalink / raw)
To: Rémi Denis-Courmont; +Cc: netdev, Rémi Denis-Courmont
In-Reply-To: <65081524fbfeb975c842ebc79a8fb038@chewa.net>
Rémi Denis-Courmont a écrit :
> It seems better than a spinlock, assuming that sockets are
> created/destroyed more seldom than they receive packets. And then
> sk_for_each_rcu does not exist. I am sure there is a good reason for that,
> though I wouldn't know. I guess I should try to use RCU hlist_nulls then?
>
spin_lock()/spin_unlock() is faster than read_lock()/read_unlock(), unless
there is contention. (two atomic ops instead of one)
So, unless you have a particular performance problem, it's actually
better to use a spinlock.
If you do have performance problem, a RCU conversion is better than rwlock.
I can do RCU conversion if you ask me...
^ permalink raw reply
* Re: [PATCH 1/2] udp: cleanup __udp4_lib_mcast_deliver
From: Lucian Adrian Grijincu @ 2009-11-06 14:04 UTC (permalink / raw)
To: David Miller; +Cc: netdev, opurdila
In-Reply-To: <20091106.004215.114490979.davem@davemloft.net>
În data de Vin 06 Noi 2009 10:42:15 David Miller a scris:
> Getting rid of the useless return value is fine, but the
> new do{}while() loop et al. is less readable to me.
It's not new.
I just moved it:
previously:
if(cond) {
do {}
while()
} else {return}
now:
if (!cond)
return;
do {}
while {}
Also this moves a consume_skb() from a path protected by spin locks.
As far as I understand it, the spin locks protect the hslot, and freeing the
skb does not walk/change or interact with the hslot in any way.
--
Lucian
^ permalink raw reply
* Re: [PATCH net-next] Phonet: use rwlock for sockets list
From: Rémi Denis-Courmont @ 2009-11-06 14:29 UTC (permalink / raw)
To: ext Eric Dumazet; +Cc: netdev@vger.kernel.org
In-Reply-To: <4AF42C07.4050008@gmail.com>
On Friday 06 November 2009 16:00:39 ext Eric Dumazet, you wrote:
> Rémi Denis-Courmont a écrit :
> > It seems better than a spinlock, assuming that sockets are
> > created/destroyed more seldom than they receive packets. And then
> > sk_for_each_rcu does not exist. I am sure there is a good reason for
> > that, though I wouldn't know. I guess I should try to use RCU hlist_nulls
> > then?
>
> spin_lock()/spin_unlock() is faster than read_lock()/read_unlock(), unless
> there is contention. (two atomic ops instead of one)
>
> So, unless you have a particular performance problem, it's actually
> better to use a spinlock.
>
> If you do have performance problem, a RCU conversion is better than rwlock.
> I can do RCU conversion if you ask me...
The most obvious current bottleneck is the sockets linked-list more so than
the locking scheme (RCU vs spinlock vs rwlock). I'll fix that soonish
regardless of the locking.
Then there is the devices lists. I did not want to clutter all net_device's
with a Phonet-specific pointer, since most devices aren't Phonet-capable. So
we have a linked-list for per-device data. There, I guess RCU would make
sense, but it did not seem trivial a change at all.
That said, there are no real "problems" yet - I was just trying to improve
things. N900 ships with the 2.6.30 Phonet stack, and it works fine :) The 3G
access network is typically the biggest bottleneck anyway...
So if you say R/W locks suck, screw this patch.
Thanks!
--
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki
^ permalink raw reply
* Re: netfilter 02/02: xt_connlimit: fix regression caused by zero family value
From: Patrick McHardy @ 2009-11-06 14:49 UTC (permalink / raw)
To: Jan Engelhardt
Cc: David S. Miller, Netfilter Development Mailinglist,
Linux Netdev List
In-Reply-To: <alpine.LSU.2.00.0911051943130.25147@obet.zrqbmnf.qr>
Jan Engelhardt wrote:
> On Thursday 2009-11-05 19:23, Patrick McHardy wrote:
>> netfilter: xt_connlimit: fix regression caused by zero family value
>>
>> Commit v2.6.28-rc1~7172~1092~2 was slightly incomplete; not all
>> instances of par->match->family were changed to par->family.
>>
>> Netfilter bugzilla #610.
>
> Hold it.
> git would never output ~7172~1092~2 because ~8266 would be much simpler.
>
> I originally wrote "Commit v2.6.28-rc1~717^2~109^2~2", but one of your
> programs seems to eat commit messages or more.
Indeed, my email client displays ^2 as ², which cut and paste transforms
to 2 :)
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" 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: [PATCH 1/2] udp: cleanup __udp4_lib_mcast_deliver
From: Eric Dumazet @ 2009-11-06 15:10 UTC (permalink / raw)
To: Lucian Adrian Grijincu; +Cc: David Miller, netdev, opurdila
In-Reply-To: <200911061604.21465.lgrijincu@ixiacom.com>
Lucian Adrian Grijincu a écrit :
>
> As far as I understand it, the spin locks protect the hslot, and freeing the
> skb does not walk/change or interact with the hslot in any way.
>
Yes, but this single skb freeing is in multicast very slow path
(it happens if we receive a multicast packet with no listener, which should
not happen with multicast aware network...)
If you really want to optimize this part, we could use an array of
32 (or 64) socket pointers, to be able to perform the really expensive
work (skb_clone(), udp_queue_rcv_skb()) outside of the lock.
Something like this untested patch :
net/ipv4/udp.c | 68 ++++++++++++++++++++++++++++++-----------------
1 files changed, 44 insertions(+), 24 deletions(-)
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index d5e75e9..5d71aee 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1190,6 +1190,24 @@ drop:
return -1;
}
+
+static void flush_stack(struct sock **stack, unsigned int count,
+ struct sk_buff *skb, unsigned int final)
+{
+ unsigned int i;
+ struct sk_buff *skb1 = NULL;
+
+ for (i = 0; i < count; i++) {
+ if (likely(skb1 == NULL))
+ skb1 = (i == final) ? skb : skb_clone(skb, GFP_ATOMIC);
+
+ if (likely(skb1 && udp_queue_rcv_skb(stack[i], skb1) <= 0))
+ skb1 = NULL;
+ }
+ if (unlikely(skb1))
+ consume_skb(skb1);
+}
+
/*
* Multicasts and broadcasts go to each listener.
*
@@ -1201,38 +1219,40 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
__be32 saddr, __be32 daddr,
struct udp_table *udptable)
{
- struct sock *sk;
+ struct sock *sk, *stack[256 / sizeof(void *)];
struct udp_hslot *hslot = udp_hashslot(udptable, net, ntohs(uh->dest));
int dif;
+ unsigned int i, count = 0;
spin_lock(&hslot->lock);
sk = sk_nulls_head(&hslot->head);
dif = skb->dev->ifindex;
sk = udp_v4_mcast_next(net, sk, uh->dest, daddr, uh->source, saddr, dif);
- if (sk) {
- struct sock *sknext = NULL;
-
- do {
- struct sk_buff *skb1 = skb;
-
- sknext = udp_v4_mcast_next(net, sk_nulls_next(sk), uh->dest,
- daddr, uh->source, saddr,
- dif);
- if (sknext)
- skb1 = skb_clone(skb, GFP_ATOMIC);
-
- if (skb1) {
- int ret = udp_queue_rcv_skb(sk, skb1);
- if (ret > 0)
- /* we should probably re-process instead
- * of dropping packets here. */
- kfree_skb(skb1);
- }
- sk = sknext;
- } while (sknext);
- } else
- consume_skb(skb);
+ while (sk) {
+ stack[count++] = sk;
+ if (unlikely(count == ARRAY_SIZE(stack))) {
+ flush_stack(stack, count, skb, ~0);
+ count = 0;
+ }
+
+ sk = udp_v4_mcast_next(net, sk_nulls_next(sk), uh->dest,
+ daddr, uh->source, saddr, dif);
+ }
+ /*
+ * before releasing the lock, we must take reference on sockets
+ */
+ for (i = 0; i < count; i++)
+ sock_hold(stack[i]);
+
spin_unlock(&hslot->lock);
+
+ /*
+ * do the slow work with no lock held
+ */
+ flush_stack(stack, count, skb, count - 1);
+
+ for (i = 0; i < count; i++)
+ sock_put(stack[i]);
return 0;
}
^ permalink raw reply related
* Re: netfilter 02/02: xt_connlimit: fix regression caused by zero family value
From: Eric Dumazet @ 2009-11-06 15:14 UTC (permalink / raw)
To: Patrick McHardy
Cc: Jan Engelhardt, David S. Miller,
Netfilter Development Mailinglist, Linux Netdev List
In-Reply-To: <4AF43782.50604@trash.net>
Patrick McHardy a écrit :
> Indeed, my email client displays ^2 as ², which cut and paste transforms
> to 2 :)
>
One more Thunderbird addict^Wvictim :)
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" 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: [RFC] netlink: add socket destruction notification
From: Patrick McHardy @ 2009-11-06 15:21 UTC (permalink / raw)
To: Johannes Berg; +Cc: netdev, Jouni Malinen, Thomas Graf
In-Reply-To: <1254473048.3959.76.camel@johannes.local>
Johannes Berg wrote:
> When we want to keep track of resources associated with applications, we
> need to know when an app is going away. Add a notification function to
> netlink that tells us that, and also hook it up to generic netlink so
> generic netlink can notify the families. Due to the way generic netlink
> works though, we need to notify all families and they have to sort out
> whatever resources some commands associated with the socket themselves.
>
> @@ -166,6 +167,9 @@ static void netlink_sock_destruct(struct
> return;
> }
>
> + if (nlk->destruct)
> + nlk->destruct(sk);
> +
> WARN_ON(atomic_read(&sk->sk_rmem_alloc));
> WARN_ON(atomic_read(&sk->sk_wmem_alloc));
> WARN_ON(nlk_sk(sk)->groups);
This seems pretty similar to the NETLINK_URELEASE notifier invoked
in netlink_release(). Wouldn't that one work as well?
^ permalink raw reply
* Re: wanPMC-CxT1E1
From: Bob Beers @ 2009-11-06 15:25 UTC (permalink / raw)
To: Greg KH; +Cc: netdev, Krzysztof Halasa
In-Reply-To: <4f6ba3b0911021339v20d7692fm52bcd5208002d6e4@mail.gmail.com>
Hi Greg,
I don't mean to seem impatient, but is cxt1e1 in staging yet?
When it is, do I 'git pull' or re-baseline, or what? I don't mean
to cause you too much extra work, but once I get the hang of
this, I should be giving more than I'm taking.
I started over with a git clone of 2.6.31.y (vs 2.6.32.rcX) and
could submit a new patch that does not have the two Makefile
errors. It compiles and I can modprobe the driver, but on
modprobe -r, or attempting to use the pmcc4cfg util, my
system locks up, maybe due to the incomplete port of the
2 INIT_WORK() calls.
I want to first fix the unload problem, then remove all the
kernel version checking stuff. After that, I thought would be
a good time to start fixing coding style issues, so that it will
not be too painful for others to look at.
BTW, where should the pmcc4cfg source code go? It needs a
change in the Makefile before it compiles correctly.
thanks,
--
-Bob Beers
^ permalink raw reply
* Re: [RFC] netlink: add socket destruction notification
From: Johannes Berg @ 2009-11-06 15:26 UTC (permalink / raw)
To: Patrick McHardy; +Cc: netdev, Jouni Malinen, Thomas Graf
In-Reply-To: <4AF43EF9.3020707@trash.net>
[-- Attachment #1: Type: text/plain, Size: 835 bytes --]
On Fri, 2009-11-06 at 16:21 +0100, Patrick McHardy wrote:
> Johannes Berg wrote:
> > When we want to keep track of resources associated with applications, we
> > need to know when an app is going away. Add a notification function to
> > netlink that tells us that, and also hook it up to generic netlink so
> > generic netlink can notify the families. Due to the way generic netlink
> > works though, we need to notify all families and they have to sort out
> > whatever resources some commands associated with the socket themselves.
> 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?
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-06 15:37 UTC (permalink / raw)
To: Johannes Berg; +Cc: netdev, Jouni Malinen, Thomas Graf
In-Reply-To: <1257521204.29454.31.camel@johannes.local>
Johannes Berg wrote:
> On Fri, 2009-11-06 at 16:21 +0100, Patrick McHardy wrote:
>> Johannes Berg wrote:
>>> When we want to keep track of resources associated with applications, we
>>> need to know when an app is going away. Add a notification function to
>>> netlink that tells us that, and also hook it up to generic netlink so
>>> generic netlink can notify the families. Due to the way generic netlink
>>> works though, we need to notify all families and they have to sort out
>>> whatever resources some commands associated with the socket themselves.
>
>
>> 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.
^ permalink raw reply
* [PATCH net-next-2.6] udp: Optimise multicast reception
From: Eric Dumazet @ 2009-11-06 15:59 UTC (permalink / raw)
To: Lucian Adrian Grijincu, David Miller; +Cc: netdev, opurdila
In-Reply-To: <4AF43C5E.4060300@gmail.com>
Eric Dumazet a écrit :
> Yes, but this single skb freeing is in multicast very slow path
> (it happens if we receive a multicast packet with no listener, which should
> not happen with multicast aware network...)
>
>
> If you really want to optimize this part, we could use an array of
> 32 (or 64) socket pointers, to be able to perform the really expensive
> work (skb_clone(), udp_queue_rcv_skb()) outside of the lock.
>
> Something like this untested patch :
I did some tests and made one fix.
With this kind of stacking, we eventually could try a rcu lookup as well.
[PATCH net-next-2.6] udp: Optimise multicast reception
UDP multicast rx path is a bit complex and can hold a spinlock
for a long time.
Using a small (32 or 64 entries) stack of socket pointers can help
to perform expensive operations (skb_clone(), udp_queue_rcv_skb())
outside of the lock, in most cases.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
net/ipv4/udp.c | 70 ++++++++++++++++++++++++++++++-----------------
1 files changed, 46 insertions(+), 24 deletions(-)
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index d5e75e9..89637d0 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1190,6 +1190,24 @@ drop:
return -1;
}
+
+static void flush_stack(struct sock **stack, unsigned int count,
+ struct sk_buff *skb, unsigned int final)
+{
+ unsigned int i;
+ struct sk_buff *skb1 = NULL;
+
+ for (i = 0; i < count; i++) {
+ if (likely(skb1 == NULL))
+ skb1 = (i == final) ? skb : skb_clone(skb, GFP_ATOMIC);
+
+ if (skb1 && udp_queue_rcv_skb(stack[i], skb1) <= 0)
+ skb1 = NULL;
+ }
+ if (skb1)
+ consume_skb(skb1);
+}
+
/*
* Multicasts and broadcasts go to each listener.
*
@@ -1201,38 +1219,42 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
__be32 saddr, __be32 daddr,
struct udp_table *udptable)
{
- struct sock *sk;
+ struct sock *sk, *stack[256 / sizeof(struct sock *)];
struct udp_hslot *hslot = udp_hashslot(udptable, net, ntohs(uh->dest));
int dif;
+ unsigned int i, count = 0;
spin_lock(&hslot->lock);
sk = sk_nulls_head(&hslot->head);
dif = skb->dev->ifindex;
sk = udp_v4_mcast_next(net, sk, uh->dest, daddr, uh->source, saddr, dif);
- if (sk) {
- struct sock *sknext = NULL;
-
- do {
- struct sk_buff *skb1 = skb;
-
- sknext = udp_v4_mcast_next(net, sk_nulls_next(sk), uh->dest,
- daddr, uh->source, saddr,
- dif);
- if (sknext)
- skb1 = skb_clone(skb, GFP_ATOMIC);
-
- if (skb1) {
- int ret = udp_queue_rcv_skb(sk, skb1);
- if (ret > 0)
- /* we should probably re-process instead
- * of dropping packets here. */
- kfree_skb(skb1);
- }
- sk = sknext;
- } while (sknext);
- } else
- consume_skb(skb);
+ while (sk) {
+ stack[count++] = sk;
+ sk = udp_v4_mcast_next(net, sk_nulls_next(sk), uh->dest,
+ daddr, uh->source, saddr, dif);
+ if (unlikely(count == ARRAY_SIZE(stack))) {
+ if (!sk)
+ break;
+ flush_stack(stack, count, skb, ~0);
+ count = 0;
+ }
+
+ }
+ /*
+ * before releasing the lock, we must take reference on sockets
+ */
+ for (i = 0; i < count; i++)
+ sock_hold(stack[i]);
+
spin_unlock(&hslot->lock);
+
+ /*
+ * do the slow work with no lock held
+ */
+ flush_stack(stack, count, skb, count - 1);
+
+ for (i = 0; i < count; i++)
+ sock_put(stack[i]);
return 0;
}
^ permalink raw reply related
* Re: [RFC PATCH] net: add dataref destructor to sk_buff
From: Gregory Haskins @ 2009-11-06 16:08 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-kernel, alacrityvm-devel@lists.sourceforge.net
In-Reply-To: <20091105.210810.124818005.davem@davemloft.net>
[-- Attachment #1: Type: text/plain, Size: 4583 bytes --]
David Miller wrote:
> From: Gregory Haskins <ghaskins@novell.com>
> Date: Fri, 02 Oct 2009 10:20:00 -0400
>
>> The following is an RFC for an attempt at addressing a zero-copy solution.
>>
>> To be perfectly honest, I have no idea if this is the best solution, or if
>> there is truly a problem with skb->destructor that requires an alternate
>> mechanism. What I do know is that this patch seems to work, and I would
>> like to see some kind of solution available upstream. So I thought I would
>> send my hack out as at least a point of discussion. FWIW: This has been
>> tested heavily in my rig and is technically suitable for inclusion after
>> review as is, if that is decided to be the optimal path forward here.
>>
>> Thanks for your review and consideration,
>
> I have no fundamental objections, but when you submit this for
> real can you show the code that uses it so we can get a better
> idea about things?
>
> Thanks.
Absolutely. I am not sure if this content would be appropriate for the
patch header, so I will just reply to your request here. If you would
like to see the patch resubmitted with the following in the header, let
me know.
The way we use this today is in the venet driver as part of the
AlacrityVM hypervisor. We therefore have a guest and host environment,
where the guest builds fully formed L2 frames, and the host generally
acts as a conduit for passing those frames to a real physical device
(such as through a soft-bridge, etc). We would like to do this without
requiring copies for certain classes of packets (i.e. packets larger
than a threshold). However we have to be smart about how we do this
since the guest technically "owns" the pages, and therefore needs
io-completion events to properly signify when pages are actually freed.
The way this all looks today is (I hope this doesn't get mangled):
----------------------------------------------------------------------
| guest | host |
----------------------------------------------------------------------
| stack | venet | venetdev | phydev |
----------------------------------------------------------------------
| alloc_skb() |
| dev_xmit() |
| -> queue_tx(sg) |
| -> dequeue_rx(sg) |
| alloc_pskb() |
| map_sg(sg)->pskb |
| loop(get_page()) |
| skb->release = cb |
| -> dev_xmit() |
| |
| txc_isr() <- |
| kfree_skb() |
| skb->release() |
| cb() <- |
| queue_event(sg) |
| txc_isr() <- |
| kfree_skb() |
| loop(put_page())|
----------------------------------------------------------------------
And here is the actual code in action
(kernel/vbus/devices/venet/device.c) from the alacrityvm.git tree
http://git.kernel.org/?p=linux/kernel/git/ghaskins/alacrityvm/linux-2.6.git;a=blob;f=kernel/vbus/devices/venet/device.c;h=d49ba7fa9f70cbb7e61c366d52d4c316d15f8b73;hb=HEAD
Line 587 is the "dequeue_rx()" operation from the diagram. Line 627 is
where we map in the guests pages to a scatterlist. Line 649 is where we
update the skb_shinfo->frags with the mapping. And finally, Line 677 is
where I register a callback for when the skb is released.
Line 853 is the callback that the stack invokes when the phydev finally
frees the packet. You can see that line 863 then sends an
transmit-complete event back up to the guest.
If this is not what you were looking for, please let me know. If this
looks acceptable to you, please consider the original patch for
inclusion at the next convenient merge window.
Thanks David!
-Greg
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 267 bytes --]
^ permalink raw reply
* Re: [PATCHv7 3/3] vhost_net: a kernel-level virtio server
From: Paul E. McKenney @ 2009-11-06 16:30 UTC (permalink / raw)
To: Rusty Russell
Cc: Michael S. Tsirkin, Gregory Haskins, Eric Dumazet, netdev,
virtualization, kvm, linux-kernel, mingo, linux-mm, akpm, hpa,
s.hetze
In-Reply-To: <200911061531.20299.rusty@rustcorp.com.au>
On Fri, Nov 06, 2009 at 03:31:20PM +1030, Rusty Russell wrote:
> On Thu, 5 Nov 2009 03:55:42 am Paul E. McKenney wrote:
> > On Wed, Nov 04, 2009 at 01:57:29PM +0200, Michael S. Tsirkin wrote:
> > > Can you ack this usage please?
> >
> > I thought I had done so in my paragraph above, but if you would like
> > something a bit more formal...
>
> <snip verbose super-ack with qualifications>
>
> That's great guys. And yes, this is a kind of read-copy-update. And no,
> there's nothing wrong with it.
>
> But it's still nasty to use half an API. If it were a few places I would
> have open-coded it with a comment, or wrapped it. As it is, I don't think
> that would be a win.
So would it help to have a rcu_read_lock_workqueue() and
rcu_read_unlock_workqueue() that checked nesting and whether they were
actually running in the context of a workqueue item? Or did you have
something else in mind? Or am I misjudging the level of sarcasm in
your reply? ;-)
Thanx, Paul
--
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
* [PATCH net-next-2.6] ipv6: udp: Optimise multicast reception
From: Eric Dumazet @ 2009-11-06 16:30 UTC (permalink / raw)
To: David Miller; +Cc: Lucian Adrian Grijincu, netdev, opurdila
In-Reply-To: <4AF447F7.6000700@gmail.com>
And here is the ipv6 part as well
[PATCH net-next-2.6] ipv6: udp: Optimise multicast reception
^ permalink raw reply
* Re: Using netconsole and getting double prints
From: Luis R. Rodriguez @ 2009-11-06 16:32 UTC (permalink / raw)
To: Matt Mackall; +Cc: linux-kernel, netdev
In-Reply-To: <1257484663.2873.622.camel@calx>
On Thu, Nov 5, 2009 at 9:17 PM, Matt Mackall <mpm@selenic.com> wrote:
> On Thu, 2009-11-05 at 16:54 -0800, Luis R. Rodriguez wrote:
>> I'm getting double prints when using netconsole. This used to happen
>> to me and then I just enable debugging log level manually (dmesg -n 8)
>> but now no matter what I try I always get double prints.
>
> This is the first report I've seen of that. Recommend investigating with
> wireshark.
Matt, did you really mean wireshark? I'm a wireless guy so wireshark
to me is this:
http://www.wireshark.org/
The prints have nothing to do with protocols though. I can get a
double print on a simple hello world driver.
Luis
^ permalink raw reply
* Re: [PATCH net-next-2.6] udp: Optimise multicast reception
From: Lucian Adrian Grijincu @ 2009-11-06 16:35 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, opurdila
In-Reply-To: <4AF447F7.6000700@gmail.com>
[-- Attachment #1: Type: Text/Plain, Size: 1298 bytes --]
În data de Vin 06 Noi 2009 17:59:51 ați scris:
> +static void flush_stack(struct sock **stack, unsigned int count,
> + struct sk_buff *skb, unsigned int final)
> +{
> + unsigned int i;
> + struct sk_buff *skb1 = NULL;
> +
> + for (i = 0; i < count; i++) {
> + if (likely(skb1 == NULL))
> + skb1 = (i == final) ? skb : skb_clone(skb, GFP_ATOMIC);
> +
> + if (skb1 && udp_queue_rcv_skb(stack[i], skb1) <= 0)
> + skb1 = NULL;
> + }
> + if (skb1)
> + consume_skb(skb1);
> +}
consume_skb() assumes the skb was successfuly transmitted.
free_skb() does the same thing, but assumes that the frame is being dropped
after a failure and notes that.
In your code, if (count == 0) you:
* fail to remove the original skb (memory leak),
* simply consume the last dropped skb, without noting the droping failure.
I fixed these in the attached (untested) patch.
One last issue: you silently ignore dropped failures (skb1 is reused in case
of a failure).
If this tracing must record all failures, I'd add an
trace_kfree_skb(skb1, __builtin_return_address(0));
if udp_queue_rcv_skb() fails.
Other than this, nicely done!
--
Lucian
[-- Attachment #2: udp-optimise-multicast-reception.patch --]
[-- Type: text/plain, Size: 2359 bytes --]
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index d5e75e9..a2c1a27 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1190,6 +1190,29 @@ drop:
return -1;
}
+
+static void flush_stack(struct sock **stack, unsigned int count,
+ struct sk_buff *skb, unsigned int final)
+{
+ unsigned int i;
+ struct sk_buff *skb1 = NULL;
+
+ if ((count == 0) && skb) {
+ consume_skb(skb);
+ return;
+ }
+
+ for (i = 0; i < count; i++) {
+ if (likely(skb1 == NULL))
+ skb1 = (i == final) ? skb : skb_clone(skb, GFP_ATOMIC);
+
+ if (skb1 && udp_queue_rcv_skb(stack[i], skb1) <= 0)
+ skb1 = NULL;
+ }
+ if (skb1)
+ free_skb(skb1);
+}
+
/*
* Multicasts and broadcasts go to each listener.
*
@@ -1201,38 +1224,42 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
__be32 saddr, __be32 daddr,
struct udp_table *udptable)
{
- struct sock *sk;
+ struct sock *sk, *stack[256 / sizeof(struct sock *)];
struct udp_hslot *hslot = udp_hashslot(udptable, net, ntohs(uh->dest));
int dif;
+ unsigned int i, count = 0;
spin_lock(&hslot->lock);
sk = sk_nulls_head(&hslot->head);
dif = skb->dev->ifindex;
sk = udp_v4_mcast_next(net, sk, uh->dest, daddr, uh->source, saddr, dif);
- if (sk) {
- struct sock *sknext = NULL;
-
- do {
- struct sk_buff *skb1 = skb;
-
- sknext = udp_v4_mcast_next(net, sk_nulls_next(sk), uh->dest,
- daddr, uh->source, saddr,
- dif);
- if (sknext)
- skb1 = skb_clone(skb, GFP_ATOMIC);
-
- if (skb1) {
- int ret = udp_queue_rcv_skb(sk, skb1);
- if (ret > 0)
- /* we should probably re-process instead
- * of dropping packets here. */
- kfree_skb(skb1);
- }
- sk = sknext;
- } while (sknext);
- } else
- consume_skb(skb);
+ while (sk) {
+ stack[count++] = sk;
+ sk = udp_v4_mcast_next(net, sk_nulls_next(sk), uh->dest,
+ daddr, uh->source, saddr, dif);
+ if (unlikely(count == ARRAY_SIZE(stack))) {
+ if (!sk)
+ break;
+ flush_stack(stack, count, skb, ~0);
+ count = 0;
+ }
+
+ }
+ /*
+ * before releasing the lock, we must take reference on sockets
+ */
+ for (i = 0; i < count; i++)
+ sock_hold(stack[i]);
+
spin_unlock(&hslot->lock);
+
+ /*
+ * do the slow work with no lock held
+ */
+ flush_stack(stack, count, skb, count - 1);
+
+ for (i = 0; i < count; i++)
+ sock_put(stack[i]);
return 0;
}
^ permalink raw reply related
* [PATCH net-next-2.6] ipv6: udp: Optimise multicast reception
From: Eric Dumazet @ 2009-11-06 16:35 UTC (permalink / raw)
To: David Miller; +Cc: Lucian Adrian Grijincu, netdev, opurdila
In-Reply-To: <4AF44F3D.3050407@gmail.com>
Sorry for previous empty mail :(
[PATCH net-next-2.6] ipv6: udp: Optimise multicast reception
IPV6 UDP multicast rx path is a bit complex and can hold a spinlock
for a long time.
Using a small (32 or 64 entries) stack of socket pointers can help
to perform expensive operations (skb_clone(), udp_queue_rcv_skb())
outside of the lock, in most cases.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
net/ipv6/udp.c | 67 ++++++++++++++++++++++++++++++-----------------
1 files changed, 43 insertions(+), 24 deletions(-)
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 5bc7cdb..670662a 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -440,6 +440,27 @@ static struct sock *udp_v6_mcast_next(struct net *net, struct sock *sk,
return NULL;
}
+static void flush_stack(struct sock **stack, unsigned int count,
+ struct sk_buff *skb, unsigned int final)
+{
+ unsigned int i;
+ struct sock *sk;
+ struct sk_buff *skb1;
+
+ for (i = 0; i < count; i++) {
+ skb1 = (i == final) ? skb : skb_clone(skb, GFP_ATOMIC);
+
+ if (skb1) {
+ sk = stack[i];
+ bh_lock_sock(sk);
+ if (!sock_owned_by_user(sk))
+ udpv6_queue_rcv_skb(sk, skb1);
+ else
+ sk_add_backlog(sk, skb1);
+ bh_unlock_sock(sk);
+ }
+ }
+}
/*
* Note: called only from the BH handler context,
* so we don't need to lock the hashes.
@@ -448,41 +469,39 @@ static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
struct in6_addr *saddr, struct in6_addr *daddr,
struct udp_table *udptable)
{
- struct sock *sk, *sk2;
+ struct sock *sk, *stack[256 / sizeof(struct sock *)];
const struct udphdr *uh = udp_hdr(skb);
struct udp_hslot *hslot = udp_hashslot(udptable, net, ntohs(uh->dest));
int dif;
+ unsigned int i, count = 0;
spin_lock(&hslot->lock);
sk = sk_nulls_head(&hslot->head);
dif = inet6_iif(skb);
sk = udp_v6_mcast_next(net, sk, uh->dest, daddr, uh->source, saddr, dif);
- if (!sk) {
- kfree_skb(skb);
- goto out;
- }
-
- sk2 = sk;
- while ((sk2 = udp_v6_mcast_next(net, sk_nulls_next(sk2), uh->dest, daddr,
- uh->source, saddr, dif))) {
- struct sk_buff *buff = skb_clone(skb, GFP_ATOMIC);
- if (buff) {
- bh_lock_sock(sk2);
- if (!sock_owned_by_user(sk2))
- udpv6_queue_rcv_skb(sk2, buff);
- else
- sk_add_backlog(sk2, buff);
- bh_unlock_sock(sk2);
+ while (sk) {
+ stack[count++] = sk;
+ sk = udp_v6_mcast_next(net, sk_nulls_next(sk), uh->dest, daddr,
+ uh->source, saddr, dif);
+ if (unlikely(count == ARRAY_SIZE(stack))) {
+ if (!sk)
+ break;
+ flush_stack(stack, count, skb, ~0);
+ count = 0;
}
}
- bh_lock_sock(sk);
- if (!sock_owned_by_user(sk))
- udpv6_queue_rcv_skb(sk, skb);
- else
- sk_add_backlog(sk, skb);
- bh_unlock_sock(sk);
-out:
+ /*
+ * before releasing the lock, we must take reference on sockets
+ */
+ for (i = 0; i < count; i++)
+ sock_hold(stack[i]);
+
spin_unlock(&hslot->lock);
+
+ flush_stack(stack, count, skb, count - 1);
+
+ for (i = 0; i < count; i++)
+ sock_put(stack[i]);
return 0;
}
^ permalink raw reply related
* Re: Using netconsole and getting double prints
From: Patrick McHardy @ 2009-11-06 16:38 UTC (permalink / raw)
To: Luis R. Rodriguez; +Cc: linux-kernel, netdev
In-Reply-To: <43e72e890911051654i19c18bf3nd397662ed79712c4@mail.gmail.com>
Luis R. Rodriguez wrote:
> I'm getting double prints when using netconsole. This used to happen
> to me and then I just enable debugging log level manually (dmesg -n 8)
> but now no matter what I try I always get double prints.
>
> For example:
>
> [ 23.425448] console [netcon0] enabled
> [ 23.425567] netconsole: network logging started
> [ 23.425448] console [netcon0] enabled
> [ 23.425567] netconsole: network logging started
> [ 32.856073] eth0: no IPv6 routers present
> [ 32.856073] eth0: no IPv6 routers present
> [ 66.307342] kmemleak: 6 new suspected memory leaks (see
> /sys/kernel/debug/kmemleak)
> [ 66.307342] kmemleak: 6 new suspected memory leaks (see
> /sys/kernel/debug/kmemleak)
>
> Instead of dmesg -8 I'm now using ignore_loglevel as a kernel
> parameter but I still get double prints. This also happens if I use
> "debug" as a kernel parameter instead.
>
> The netconsole is set up on the dev box as follows through an /etc/rc.local
>
> dhclient eth0
> IP=192.168.2
> # sudo dmesg -n 8
> modprobe netconsole netconsole=@/eth0,6666@${IP}/
Without a MAC address netconsole broadcasts the messages.
So I'd guess the receiving system has multiple NICs and
receives the message multiple times.
^ permalink raw reply
* Re: Using netconsole and getting double prints
From: Matt Mackall @ 2009-11-06 16:43 UTC (permalink / raw)
To: Luis R. Rodriguez; +Cc: linux-kernel, netdev
In-Reply-To: <43e72e890911060832k1b831f32w93357c71b370e71c@mail.gmail.com>
On Fri, 2009-11-06 at 08:32 -0800, Luis R. Rodriguez wrote:
> On Thu, Nov 5, 2009 at 9:17 PM, Matt Mackall <mpm@selenic.com> wrote:
> > On Thu, 2009-11-05 at 16:54 -0800, Luis R. Rodriguez wrote:
> >> I'm getting double prints when using netconsole. This used to happen
> >> to me and then I just enable debugging log level manually (dmesg -n 8)
> >> but now no matter what I try I always get double prints.
> >
> > This is the first report I've seen of that. Recommend investigating with
> > wireshark.
>
> Matt, did you really mean wireshark? I'm a wireless guy so wireshark
> to me is this:
>
> http://www.wireshark.org/
> The prints have nothing to do with protocols though. I can get a
> double print on a simple hello world driver.
Are you saying you get these double prints on the *local* machine? Are
the messages logged dmesg doubled too?
--
http://selenic.com : development and support for Mercurial and Linux
^ permalink raw reply
* Re: [PATCH net-next2.6] ipv6: avoid dev_hold()/dev_put() in rawv6_bind()
From: Brian Haley @ 2009-11-06 16:48 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S. Miller, Linux Netdev List
In-Reply-To: <4AF40022.6050100@gmail.com>
Eric Dumazet wrote:
> @@ -257,6 +257,7 @@ static int rawv6_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len)
> if (sk->sk_state != TCP_CLOSE)
> goto out;
>
> + rcu_read_unlock();
> /* Check if the address belongs to the host. */
> if (addr_type != IPV6_ADDR_ANY) {
> struct net_device *dev = NULL;
This was supposed to be rcu_read_lock(), right?
-Brian
^ permalink raw reply
* Re: [PATCH net-next-2.6] udp: Optimise multicast reception
From: Eric Dumazet @ 2009-11-06 16:54 UTC (permalink / raw)
To: Lucian Adrian Grijincu; +Cc: David Miller, netdev, opurdila
In-Reply-To: <200911061835.24928.lgrijincu@ixiacom.com>
Lucian Adrian Grijincu a écrit :
> În data de Vin 06 Noi 2009 17:59:51 ați scris:
>> +static void flush_stack(struct sock **stack, unsigned int count,
>> + struct sk_buff *skb, unsigned int final)
>> +{
>> + unsigned int i;
>> + struct sk_buff *skb1 = NULL;
>> +
>> + for (i = 0; i < count; i++) {
>> + if (likely(skb1 == NULL))
>> + skb1 = (i == final) ? skb : skb_clone(skb, GFP_ATOMIC);
>> +
>> + if (skb1 && udp_queue_rcv_skb(stack[i], skb1) <= 0)
>> + skb1 = NULL;
>> + }
>> + if (skb1)
>> + consume_skb(skb1);
>> +}
>
> consume_skb() assumes the skb was successfuly transmitted.
>
> free_skb() does the same thing, but assumes that the frame is being dropped
> after a failure and notes that.
>
> In your code, if (count == 0) you:
> * fail to remove the original skb (memory leak),
> * simply consume the last dropped skb, without noting the droping failure.
>
> I fixed these in the attached (untested) patch.
>
> One last issue: you silently ignore dropped failures (skb1 is reused in case
> of a failure).
>
> If this tracing must record all failures, I'd add an
> trace_kfree_skb(skb1, __builtin_return_address(0));
> if udp_queue_rcv_skb() fails.
>
> Other than this, nicely done!
>
Thanks !
Note, free_skb() doesnt exist ;)
And we should not call consume_skb() in this path.
I made the if (count == 0) done at the end of __udp4_lib_mcast_deliver()
[PATCH net-next-2.6] udp: Optimise multicast reception
UDP multicast rx path is a bit complex and can hold a spinlock
for a long time.
Using a small (32 or 64 entries) stack of socket pointers can help
to perform expensive operations (skb_clone(), udp_queue_rcv_skb())
outside of the lock, in most cases.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Lucian Adrian Grijincu <lgrijincu@ixiacom.com>
---
net/ipv4/udp.c | 76 ++++++++++++++++++++++++++++++-----------------
1 files changed, 50 insertions(+), 26 deletions(-)
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index d5e75e9..45c73b1 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1190,49 +1190,73 @@ drop:
return -1;
}
+
+static void flush_stack(struct sock **stack, unsigned int count,
+ struct sk_buff *skb, unsigned int final)
+{
+ unsigned int i;
+ struct sk_buff *skb1 = NULL;
+
+ for (i = 0; i < count; i++) {
+ if (likely(skb1 == NULL))
+ skb1 = (i == final) ? skb : skb_clone(skb, GFP_ATOMIC);
+
+ if (skb1 && udp_queue_rcv_skb(stack[i], skb1) <= 0)
+ skb1 = NULL;
+ }
+ if (unlikely(skb1))
+ kfree_skb(skb1);
+}
+
/*
* Multicasts and broadcasts go to each listener.
*
- * Note: called only from the BH handler context,
- * so we don't need to lock the hashes.
+ * Note: called only from the BH handler context.
*/
static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
struct udphdr *uh,
__be32 saddr, __be32 daddr,
struct udp_table *udptable)
{
- struct sock *sk;
+ struct sock *sk, *stack[256 / sizeof(struct sock *)];
struct udp_hslot *hslot = udp_hashslot(udptable, net, ntohs(uh->dest));
int dif;
+ unsigned int i, count = 0;
spin_lock(&hslot->lock);
sk = sk_nulls_head(&hslot->head);
dif = skb->dev->ifindex;
sk = udp_v4_mcast_next(net, sk, uh->dest, daddr, uh->source, saddr, dif);
- if (sk) {
- struct sock *sknext = NULL;
-
- do {
- struct sk_buff *skb1 = skb;
-
- sknext = udp_v4_mcast_next(net, sk_nulls_next(sk), uh->dest,
- daddr, uh->source, saddr,
- dif);
- if (sknext)
- skb1 = skb_clone(skb, GFP_ATOMIC);
-
- if (skb1) {
- int ret = udp_queue_rcv_skb(sk, skb1);
- if (ret > 0)
- /* we should probably re-process instead
- * of dropping packets here. */
- kfree_skb(skb1);
- }
- sk = sknext;
- } while (sknext);
- } else
- consume_skb(skb);
+ while (sk) {
+ stack[count++] = sk;
+ sk = udp_v4_mcast_next(net, sk_nulls_next(sk), uh->dest,
+ daddr, uh->source, saddr, dif);
+ if (unlikely(count == ARRAY_SIZE(stack))) {
+ if (!sk)
+ break;
+ flush_stack(stack, count, skb, ~0);
+ count = 0;
+ }
+ }
+ /*
+ * before releasing chain lock, we must take a reference on sockets
+ */
+ for (i = 0; i < count; i++)
+ sock_hold(stack[i]);
+
spin_unlock(&hslot->lock);
+
+ /*
+ * do the slow work with no lock held
+ */
+ if (count) {
+ flush_stack(stack, count, skb, count - 1);
+
+ for (i = 0; i < count; i++)
+ sock_put(stack[i]);
+ } else {
+ kfree_skb(skb);
+ }
return 0;
}
^ permalink raw reply related
* Re: [PATCH net-next2.6] ipv6: avoid dev_hold()/dev_put() in rawv6_bind()
From: Eric Dumazet @ 2009-11-06 17:01 UTC (permalink / raw)
To: Brian Haley; +Cc: David S. Miller, Linux Netdev List
In-Reply-To: <4AF4536E.7090803@hp.com>
Brian Haley a écrit :
> Eric Dumazet wrote:
>> @@ -257,6 +257,7 @@ static int rawv6_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len)
>> if (sk->sk_state != TCP_CLOSE)
>> goto out;
>>
>> + rcu_read_unlock();
>> /* Check if the address belongs to the host. */
>> if (addr_type != IPV6_ADDR_ANY) {
>> struct net_device *dev = NULL;
>
> This was supposed to be rcu_read_lock(), right?
>
Oops ! Indeed ! Thanks for spotting this Brian.
[PATCH net-next2.6] ipv6: avoid dev_hold()/dev_put() in rawv6_bind()
Using RCU helps not touching device refcount in rawv6_bind()
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
net/ipv6/raw.c | 23 +++++++++++------------
1 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
index 818ef21..926ce8e 100644
--- a/net/ipv6/raw.c
+++ b/net/ipv6/raw.c
@@ -249,7 +249,7 @@ static int rawv6_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len)
/* Raw sockets are IPv6 only */
if (addr_type == IPV6_ADDR_MAPPED)
- return(-EADDRNOTAVAIL);
+ return -EADDRNOTAVAIL;
lock_sock(sk);
@@ -257,6 +257,7 @@ static int rawv6_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len)
if (sk->sk_state != TCP_CLOSE)
goto out;
+ rcu_read_lock();
/* Check if the address belongs to the host. */
if (addr_type != IPV6_ADDR_ANY) {
struct net_device *dev = NULL;
@@ -272,13 +273,13 @@ static int rawv6_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len)
/* Binding to link-local address requires an interface */
if (!sk->sk_bound_dev_if)
- goto out;
+ goto out_unlock;
- dev = dev_get_by_index(sock_net(sk), sk->sk_bound_dev_if);
- if (!dev) {
- err = -ENODEV;
- goto out;
- }
+ err = -ENODEV;
+ dev = dev_get_by_index_rcu(sock_net(sk),
+ sk->sk_bound_dev_if);
+ if (!dev)
+ goto out_unlock;
}
/* ipv4 addr of the socket is invalid. Only the
@@ -289,13 +290,9 @@ static int rawv6_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len)
err = -EADDRNOTAVAIL;
if (!ipv6_chk_addr(sock_net(sk), &addr->sin6_addr,
dev, 0)) {
- if (dev)
- dev_put(dev);
- goto out;
+ goto out_unlock;
}
}
- if (dev)
- dev_put(dev);
}
inet->inet_rcv_saddr = inet->inet_saddr = v4addr;
@@ -303,6 +300,8 @@ static int rawv6_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len)
if (!(addr_type & IPV6_ADDR_MULTICAST))
ipv6_addr_copy(&np->saddr, &addr->sin6_addr);
err = 0;
+out_unlock:
+ rcu_read_unlock();
out:
release_sock(sk);
return err;
^ 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