* [PATCH] cli/sti in net/802/*
@ 2002-07-22 19:48 Petr Vandrovec
2002-07-22 19:59 ` Petr Vandrovec
2002-07-23 2:07 ` David S. Miller
0 siblings, 2 replies; 4+ messages in thread
From: Petr Vandrovec @ 2002-07-22 19:48 UTC (permalink / raw)
To: davem; +Cc: linux-kernel, mingo
Hi Dave, hi Ingo,
patch below switches 802.2 and SNAP layer to use rwlock instead
of cli/sti. If you think that read_lock() has too much overhead
in this code path, I'm sorry, I have no better solution...
It also fixes race between two tasks doing register_*_client
at once - there was no locking here.
If you agree, tell me and I'll forward it to Linus. Or do so
yourself.
Thanks,
Petr Vandrovec
vandrove@vc.cvut.cz
diff -urdN linux/net/802/p8022.c linux/net/802/p8022.c
--- linux/net/802/p8022.c 2002-07-22 13:32:52.000000000 +0000
+++ linux/net/802/p8022.c 2002-07-22 19:32:20.000000000 +0000
@@ -31,6 +31,8 @@
extern void llc_unregister_sap(unsigned char sap);
static struct datalink_proto *p8022_list;
+static rwlock_t p8022_list_lock = RW_LOCK_UNLOCKED;
+
/*
* We don't handle the loopback SAP stuff, the extended
* 802.2 command set, multicast SAP identifiers and non UI
@@ -53,6 +55,7 @@
struct datalink_proto *proto;
int rc = 0;
+ read_lock(&p8022_list_lock);
proto = find_8022_client(*(skb->h.raw));
if (!proto) {
skb->sk = NULL;
@@ -63,7 +66,8 @@
skb->nh.raw += 3;
skb_pull(skb, 3);
rc = proto->rcvfunc(skb, dev, pt);
-out: return rc;
+out: read_unlock(&p8022_list_lock);
+ return rc;
}
static void p8022_datalink_header(struct datalink_proto *dl,
@@ -84,7 +88,9 @@
struct packet_type *))
{
struct datalink_proto *proto = NULL;
+ unsigned long flags;
+ write_lock_irqsave(&p8022_list_lock, flags);
if (find_8022_client(type))
goto out;
proto = kmalloc(sizeof(*proto), GFP_ATOMIC);
@@ -99,7 +105,8 @@
p8022_list = proto;
llc_register_sap(type, p8022_rcv);
}
-out: return proto;
+out: write_unlock_irqrestore(&p8022_list_lock, flags);
+ return proto;
}
void unregister_8022_client(unsigned char type)
@@ -107,8 +114,7 @@
struct datalink_proto *tmp, **clients = &p8022_list;
unsigned long flags;
- save_flags(flags);
- cli();
+ write_lock_irqsave(&p8022_list_lock, flags);
while (*clients) {
tmp = *clients;
if (tmp->type[0] == type) {
@@ -119,7 +125,7 @@
}
clients = &tmp->next;
}
- restore_flags(flags);
+ write_unlock_irqrestore(&p8022_list_lock, flags);
}
EXPORT_SYMBOL(register_8022_client);
diff -urdN linux/net/802/psnap.c linux/net/802/psnap.c
--- linux/net/802/psnap.c 2002-07-22 13:32:58.000000000 +0000
+++ linux/net/802/psnap.c 2002-07-22 19:36:43.000000000 +0000
@@ -22,6 +22,7 @@
static struct datalink_proto *snap_list = NULL;
static struct datalink_proto *snap_dl = NULL; /* 802.2 DL for SNAP */
+static rwlock_t snap_list_lock = RW_LOCK_UNLOCKED;
/*
* Find a snap client by matching the 5 bytes.
@@ -52,9 +53,12 @@
struct datalink_proto *proto;
+ read_lock(&snap_list_lock);
proto = find_snap_client(skb->h.raw);
if (proto != NULL)
{
+ int err;
+
/*
* Pass the frame on.
*/
@@ -65,7 +69,9 @@
if (psnap_packet_type.type == 0)
psnap_packet_type.type=htons(ETH_P_SNAP);
- return proto->rcvfunc(skb, dev, &psnap_packet_type);
+ err = proto->rcvfunc(skb, dev, &psnap_packet_type);
+ read_unlock(&snap_list_lock);
+ return err;
}
skb->sk = NULL;
kfree_skb(skb);
@@ -104,10 +110,12 @@
struct datalink_proto *register_snap_client(unsigned char *desc, int (*rcvfunc)(struct sk_buff *, struct net_device *, struct packet_type *))
{
- struct datalink_proto *proto;
+ struct datalink_proto *proto = NULL;
+ unsigned long flags;
+ write_lock_irqsave(&snap_list_lock, flags);
if (find_snap_client(desc) != NULL)
- return NULL;
+ goto out;
proto = (struct datalink_proto *) kmalloc(sizeof(*proto), GFP_ATOMIC);
if (proto != NULL)
@@ -121,7 +129,7 @@
proto->next = snap_list;
snap_list = proto;
}
-
+out: write_unlock_irqrestore(&snap_list_lock, flags);
return proto;
}
@@ -135,8 +143,7 @@
struct datalink_proto *tmp;
unsigned long flags;
- save_flags(flags);
- cli();
+ write_lock_irqsave(&snap_list_lock, flags);
while ((tmp = *clients) != NULL)
{
@@ -150,5 +157,5 @@
clients = &tmp->next;
}
- restore_flags(flags);
+ write_unlock_irqrestore(&snap_list_lock, flags);
}
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] cli/sti in net/802/*
2002-07-22 19:48 Petr Vandrovec
@ 2002-07-22 19:59 ` Petr Vandrovec
2002-07-23 2:07 ` David S. Miller
1 sibling, 0 replies; 4+ messages in thread
From: Petr Vandrovec @ 2002-07-22 19:59 UTC (permalink / raw)
To: davem; +Cc: linux-kernel, mingo
On Mon, Jul 22, 2002 at 09:48:14PM +0200, Petr Vandrovec wrote:
> Hi Dave, hi Ingo,
> patch below switches 802.2 and SNAP layer to use rwlock instead
> of cli/sti. If you think that read_lock() has too much overhead
> in this code path, I'm sorry, I have no better solution...
>
> It also fixes race between two tasks doing register_*_client
> at once - there was no locking here.
>
> If you agree, tell me and I'll forward it to Linus. Or do so
> yourself.
> Thanks,
> Petr Vandrovec
> vandrove@vc.cvut.cz
Oops. Older version was missing read_unlock in one of psnap.c error
paths. Fixed version follows.
Petr Vandrovec
diff -urdN linux/net/802/p8022.c linux/net/802/p8022.c
--- linux/net/802/p8022.c 2002-07-22 13:32:52.000000000 +0000
+++ linux/net/802/p8022.c 2002-07-22 19:32:20.000000000 +0000
@@ -31,6 +31,8 @@
extern void llc_unregister_sap(unsigned char sap);
static struct datalink_proto *p8022_list;
+static rwlock_t p8022_list_lock = RW_LOCK_UNLOCKED;
+
/*
* We don't handle the loopback SAP stuff, the extended
* 802.2 command set, multicast SAP identifiers and non UI
@@ -53,6 +55,7 @@
struct datalink_proto *proto;
int rc = 0;
+ read_lock(&p8022_list_lock);
proto = find_8022_client(*(skb->h.raw));
if (!proto) {
skb->sk = NULL;
@@ -63,7 +66,8 @@
skb->nh.raw += 3;
skb_pull(skb, 3);
rc = proto->rcvfunc(skb, dev, pt);
-out: return rc;
+out: read_unlock(&p8022_list_lock);
+ return rc;
}
static void p8022_datalink_header(struct datalink_proto *dl,
@@ -84,7 +88,9 @@
struct packet_type *))
{
struct datalink_proto *proto = NULL;
+ unsigned long flags;
+ write_lock_irqsave(&p8022_list_lock, flags);
if (find_8022_client(type))
goto out;
proto = kmalloc(sizeof(*proto), GFP_ATOMIC);
@@ -99,7 +105,8 @@
p8022_list = proto;
llc_register_sap(type, p8022_rcv);
}
-out: return proto;
+out: write_unlock_irqrestore(&p8022_list_lock, flags);
+ return proto;
}
void unregister_8022_client(unsigned char type)
@@ -107,8 +114,7 @@
struct datalink_proto *tmp, **clients = &p8022_list;
unsigned long flags;
- save_flags(flags);
- cli();
+ write_lock_irqsave(&p8022_list_lock, flags);
while (*clients) {
tmp = *clients;
if (tmp->type[0] == type) {
@@ -119,7 +125,7 @@
}
clients = &tmp->next;
}
- restore_flags(flags);
+ write_unlock_irqrestore(&p8022_list_lock, flags);
}
EXPORT_SYMBOL(register_8022_client);
diff -urdN linux/net/802/psnap.c linux/net/802/psnap.c
--- linux/net/802/psnap.c 2002-07-22 13:32:58.000000000 +0000
+++ linux/net/802/psnap.c 2002-07-22 19:52:11.000000000 +0000
@@ -22,6 +22,7 @@
static struct datalink_proto *snap_list = NULL;
static struct datalink_proto *snap_dl = NULL; /* 802.2 DL for SNAP */
+static rwlock_t snap_list_lock = RW_LOCK_UNLOCKED;
/*
* Find a snap client by matching the 5 bytes.
@@ -52,9 +53,12 @@
struct datalink_proto *proto;
+ read_lock(&snap_list_lock);
proto = find_snap_client(skb->h.raw);
if (proto != NULL)
{
+ int err;
+
/*
* Pass the frame on.
*/
@@ -65,8 +69,11 @@
if (psnap_packet_type.type == 0)
psnap_packet_type.type=htons(ETH_P_SNAP);
- return proto->rcvfunc(skb, dev, &psnap_packet_type);
+ err = proto->rcvfunc(skb, dev, &psnap_packet_type);
+ read_unlock(&snap_list_lock);
+ return err;
}
+ read_unlock(&snap_list_lock);
skb->sk = NULL;
kfree_skb(skb);
return 0;
@@ -104,10 +111,12 @@
struct datalink_proto *register_snap_client(unsigned char *desc, int (*rcvfunc)(struct sk_buff *, struct net_device *, struct packet_type *))
{
- struct datalink_proto *proto;
+ struct datalink_proto *proto = NULL;
+ unsigned long flags;
+ write_lock_irqsave(&snap_list_lock, flags);
if (find_snap_client(desc) != NULL)
- return NULL;
+ goto out;
proto = (struct datalink_proto *) kmalloc(sizeof(*proto), GFP_ATOMIC);
if (proto != NULL)
@@ -121,7 +130,7 @@
proto->next = snap_list;
snap_list = proto;
}
-
+out: write_unlock_irqrestore(&snap_list_lock, flags);
return proto;
}
@@ -135,8 +144,7 @@
struct datalink_proto *tmp;
unsigned long flags;
- save_flags(flags);
- cli();
+ write_lock_irqsave(&snap_list_lock, flags);
while ((tmp = *clients) != NULL)
{
@@ -150,5 +158,5 @@
clients = &tmp->next;
}
- restore_flags(flags);
+ write_unlock_irqrestore(&snap_list_lock, flags);
}
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] cli/sti in net/802/*
2002-07-22 19:48 Petr Vandrovec
2002-07-22 19:59 ` Petr Vandrovec
@ 2002-07-23 2:07 ` David S. Miller
1 sibling, 0 replies; 4+ messages in thread
From: David S. Miller @ 2002-07-23 2:07 UTC (permalink / raw)
To: vandrove; +Cc: linux-kernel, mingo
These patches don't make any sense.
You aren't blocking against other things that cli/sti used
to disable, namely timers and the generic input packet processing
engine.
There is no way these changes are a correct replacement for cli/sti.
This goes for your IPX changes too which I ask that you had pass
through Arnaldo de Melo in the future as he has done a lot of work
in this area.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] cli/sti in net/802/*
@ 2002-07-23 10:29 Petr Vandrovec
0 siblings, 0 replies; 4+ messages in thread
From: Petr Vandrovec @ 2002-07-23 10:29 UTC (permalink / raw)
To: David S. Miller; +Cc: linux-kernel, acme
On 22 Jul 02 at 19:07, David S. Miller wrote:
>
> These patches don't make any sense.
>
> You aren't blocking against other things that cli/sti used
> to disable, namely timers and the generic input packet processing
> engine.
>
> There is no way these changes are a correct replacement for cli/sti.
> This goes for your IPX changes too which I ask that you had pass
> through Arnaldo de Melo in the future as he has done a lot of work
> in this area.
Why? I'm definitely sure that IPX patches are correct: only
place which accesses spx_family_ops variable is ipx_create.
If we have SPX sockets created when we call ipx_unregister_spx, we
have much worse problem, because of regardless of any cli/sti, we are
going to release af_spx memory very soon, and cli does not force us to
close all SPX sockets, does it?
As for p8022/psnap changes, yes, I missed datalink_header locking.
But because of IPX uses dl->datalink_header() happilly from process
context without any locking, I thought that users of proto structure
returned from register_* are responsible for making sure that they'll
not use it anymore when they call unregister_*. Adding (any) lock
into *_datalink_header will not help unless you will call find_*_client
in *_datalink_header after obtaining lock to revalidate pointer you
received from caller. Which is obviously wrong, I'd say. And ipx
requires you to down all interfaces/close all sockets before it will
call unregister_*_client, so IPX is safe here (at least as it was always;
if kernel can send packets to downed interface, it is completely another
problem).
Thanks for explanation,
Petr Vandrovec
vandrove@vc.cvut.cz
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2002-07-23 10:26 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2002-07-23 10:29 [PATCH] cli/sti in net/802/* Petr Vandrovec
-- strict thread matches above, loose matches on Subject: below --
2002-07-22 19:48 Petr Vandrovec
2002-07-22 19:59 ` Petr Vandrovec
2002-07-23 2:07 ` David S. Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox