Re: [PATCH] af_pppox: create module infrastructure for protocol modules

Re: [PATCH] af_pppox: create module infrastructure for protocol modules

[David S. Miller]

   From: Arnaldo Carvalho de Melo <acme@conectiva.com.br>
   Date: Tue, 29 Apr 2003 03:12:27 -0300

   	Max, take a look and see if this same approach can be used in
   bluetooth, I bet it can, its just a matter of not using struct
   net_proto_family for bt_proto, just like pppox already was doing
   before my changes :-)

Something similar can be done for ipv4/ipv6 by adding a struct module
*owner member to struct inet_protosw etc. etc.

Although the idea is conceptually sound, you miss one crucial thing.
Such struct sock's reference _TWO_ modules, the "PPPOE" module
and the "PPPOX" module.

So in the TCP/UDP/SCTP example case, a struct sock references the
TCP/UDP/SCTP module _AND_ the ipv4/ipv6 module.

So what we'll need to do is use two owner pointers in struct sock,
one for propagating the "struct socket" owner, and one for the
"sub-protocol".

	struct module	*owner;
	struct module	*sub_owner;

Re: [PATCH] af_pppox: create module infrastructure for protocol modules

[David S. Miller]

   From: Arnaldo Carvalho de Melo <acme@conectiva.com.br>
   Date: Tue, 29 Apr 2003 03:54:19 -0300

   Em Mon, Apr 28, 2003 at 10:27:28PM -0700, David S. Miller escreveu:
   > Although the idea is conceptually sound, you miss one crucial thing.
   > Such struct sock's reference _TWO_ modules, the "PPPOE" module
   > and the "PPPOX" module.
   
   But what is the problem? at pppox_sk_alloc time I bump the PPPOE
   module refcnt, making it safe, then it calls sk_alloc where it
   bumps the PPPOX module, making it safe as well, so I'm taking care
   of both PPPOE and PPPOX.

You're absolutely correct, I missed this.

I'll pull your changes, thanks.

Re: [PATCH] af_pppox: create module infrastructure for protocol modules

[David S. Miller]

   From: YOSHIFUJI Hideaki / 吉藤英明 <yoshfuji@wide.ad.jp>
   Date: Tue, 29 Apr 2003 15:46:51 +0900 (JST)

   We probably need to refer IPv4 module as well as IPv6 module 
   because of the IPv4-Mapped address feature for IPv6 sockets.
   
Right.

[PATCH] af_pppox: create module infrastructure for protocol modules

[Arnaldo Carvalho de Melo]

Hi David,

	Please consider pulling from:

bk://kernel.bkbits.net/acme/net-2.5

	There is just this outstanding changeset in this tree.

	Max, take a look and see if this same approach can be used in
bluetooth, I bet it can, its just a matter of not using struct net_proto_family
for bt_proto, just like pppox already was doing before my changes :-)

- Arnaldo

You can import this changeset into BK by piping this whole message to:
'| bk receive [path to repository]' or apply the patch as usual.

===================================================================


ChangeSet@1.1147, 2003-04-29 02:55:39-03:00, acme@conectiva.com.br
  o af_pppox: create module infrastructure for protocol modules
  
  With this the pppox module is protected by the networking core and
  the pppox "core" protects modules for specific pppox protocols (pppoe,
  for instance), while doing it removed some not needed struct sock
  member initializations in pppoe_create that are done by sock_init_data.


 drivers/net/pppoe.c      |   30 ++++++++++--------------------
 drivers/net/pppox.c      |   47 ++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/if_pppox.h |   14 +++++++++++---
 3 files changed, 67 insertions(+), 24 deletions(-)


diff -Nru a/drivers/net/pppoe.c b/drivers/net/pppoe.c
--- a/drivers/net/pppoe.c	Tue Apr 29 02:58:32 2003
+++ b/drivers/net/pppoe.c	Tue Apr 29 02:58:32 2003
@@ -471,16 +471,15 @@
 
 /***********************************************************************
  *
- * Really kill the socket. (Called from sock_put if refcnt == 0.)
+ * Really kill the socket. (Called from pppox_sk_free if refcnt == 0.)
  *
  **********************************************************************/
-void pppoe_sock_destruct(struct sock *sk)
+static void pppoe_sk_free(struct sock *sk)
 {
 	struct pppox_opt *po = pppox_sk(sk);
 
 	if (po)
 		kfree(po);
-	MOD_DEC_USE_COUNT;
 }
 
 
@@ -495,26 +494,16 @@
 	struct sock *sk;
 	struct pppox_opt *po;
 
-	MOD_INC_USE_COUNT;
-
-	sk = sk_alloc(PF_PPPOX, GFP_KERNEL, 1, NULL);
+	sk = pppox_sk_alloc(sock, PX_PROTO_OE, GFP_KERNEL, 1, NULL);
 	if (!sk)
-		goto decmod;
-
-	sock_init_data(sock, sk);
+		goto out;
 
 	sock->state = SS_UNCONNECTED;
 	sock->ops   = &pppoe_ops;
 
-	sk->protocol = PX_PROTO_OE;
-	sk->family = PF_PPPOX;
-
 	sk->backlog_rcv = pppoe_rcv_core;
-	sk->next = NULL;
-	sk->pprev = NULL;
 	sk->state = PPPOX_NONE;
 	sk->type = SOCK_STREAM;
-	sk->destruct = pppoe_sock_destruct;
 
 	po = pppox_sk(sk) = kmalloc(sizeof(*po), GFP_KERNEL);
 	if (!po)
@@ -522,10 +511,8 @@
 	memset(po, 0, sizeof(*po));
 	po->sk = sk;
 	error = 0;
-	sock->sk = sk;
 out:	return error;
 frees:	sk_free(sk);
-decmod:	MOD_DEC_USE_COUNT;
 	goto out;
 }
 
@@ -1075,16 +1062,16 @@
 };
 #endif /* CONFIG_PROC_FS */
 
+/* ->release and ->ioctl are set at pppox_create */
+
 struct proto_ops pppoe_ops = {
     .family		= AF_PPPOX,
-    .release		= pppoe_release,
     .bind		= sock_no_bind,
     .connect		= pppoe_connect,
     .socketpair		= sock_no_socketpair,
     .accept		= sock_no_accept,
     .getname		= pppoe_getname,
     .poll		= datagram_poll,
-    .ioctl		= pppoe_ioctl,
     .listen		= sock_no_listen,
     .shutdown		= sock_no_shutdown,
     .setsockopt		= sock_no_setsockopt,
@@ -1096,7 +1083,10 @@
 
 struct pppox_proto pppoe_proto = {
     .create	= pppoe_create,
-    .ioctl	= pppoe_ioctl
+    .ioctl	= pppoe_ioctl,
+    .release	= pppoe_release,
+    .sk_free	= pppoe_sk_free,
+    .owner	= THIS_MODULE,
 };
 
 
diff -Nru a/drivers/net/pppox.c b/drivers/net/pppox.c
--- a/drivers/net/pppox.c	Tue Apr 29 02:58:32 2003
+++ b/drivers/net/pppox.c	Tue Apr 29 02:58:32 2003
@@ -64,9 +64,45 @@
 	}
 }
 
+static int pppox_release(struct socket *sock)
+{
+	struct sock *sk = sock->sk;
+	int rc = pppox_protos[sk->protocol]->release(sock);
+
+	module_put(pppox_protos[sk->protocol]->owner);
+	return rc;
+}
+
+static void pppox_sk_free(struct sock *sk)
+{
+	pppox_protos[sk->protocol]->sk_free(sk);
+	module_put(pppox_protos[sk->protocol]->owner);
+}
+
+struct sock *pppox_sk_alloc(struct socket *sock, int protocol, int priority,
+			    int zero_it, kmem_cache_t *slab)
+{
+	struct sock *sk = NULL;
+
+	if (!try_module_get(pppox_protos[protocol]->owner))
+		goto out;
+
+	sk = sk_alloc(PF_PPPOX, priority, zero_it, slab);
+	if (sk) {
+		sock_init_data(sock, sk);
+		sk->family   = PF_PPPOX;
+		sk->protocol = protocol;
+		sk->destruct = pppox_sk_free;
+	} else
+		module_put(pppox_protos[protocol]->owner);
+out:
+	return sk;
+}
+
 EXPORT_SYMBOL(register_pppox_proto);
 EXPORT_SYMBOL(unregister_pppox_proto);
 EXPORT_SYMBOL(pppox_unbind_sock);
+EXPORT_SYMBOL(pppox_sk_alloc);
 
 static int pppox_ioctl(struct socket* sock, unsigned int cmd, 
 		       unsigned long arg)
@@ -116,11 +152,19 @@
 	if (!pppox_protos[protocol])
 		goto out;
 
+	rc = -EBUSY;
+	if (!try_module_get(pppox_protos[protocol]->owner))
+		goto out;
+
 	rc = pppox_protos[protocol]->create(sock);
-	if (!rc)
+	if (!rc) {
 		/* We get to set the ioctl handler. */
+		/* And the release handler, for module refcounting */
 		/* For everything else, pppox is just a shell. */
 		sock->ops->ioctl = pppox_ioctl;
+		sock->ops->release = pppox_release;
+	} else
+		module_put(pppox_protos[protocol]->owner);
 out:
 	return rc;
 }
@@ -128,6 +172,7 @@
 static struct net_proto_family pppox_proto_family = {
 	.family	= PF_PPPOX,
 	.create	= pppox_create,
+	.owner	= THIS_MODULE,
 };
 
 static int __init pppox_init(void)
diff -Nru a/include/linux/if_pppox.h b/include/linux/if_pppox.h
--- a/include/linux/if_pppox.h	Tue Apr 29 02:58:32 2003
+++ b/include/linux/if_pppox.h	Tue Apr 29 02:58:32 2003
@@ -134,10 +134,15 @@
 
 #define pppox_sk(__sk) ((struct pppox_opt *)(__sk)->protinfo)
 
+struct module;
+
 struct pppox_proto {
-	int (*create)(struct socket *sock);
-	int (*ioctl)(struct socket *sock, unsigned int cmd,
-		     unsigned long arg);
+	int		(*create)(struct socket *sock);
+	int		(*ioctl)(struct socket *sock, unsigned int cmd,
+				 unsigned long arg);
+	int		(*release)(struct socket *sock);
+	void            (*sk_free)(struct sock *sk);
+	struct module	*owner;
 };
 
 extern int register_pppox_proto(int proto_num, struct pppox_proto *pp);
@@ -145,6 +150,9 @@
 extern void pppox_unbind_sock(struct sock *sk);/* delete ppp-channel binding */
 extern int pppox_channel_ioctl(struct ppp_channel *pc, unsigned int cmd,
 			       unsigned long arg);
+extern struct sock *pppox_sk_alloc(struct socket *sock, int protocol,
+				   int priority, int zero_it,
+				   kmem_cache_t *slab);
 
 /* PPPoX socket states */
 enum {

===================================================================


This BitKeeper patch contains the following changesets:
1.1147
## Wrapped with gzip_uu ##


M'XL( (@4KCX  ^U86W/:.!1^MG^%MGTA+!?)\A6&3C>%MIFFA4F:V7:V.QXC
MB^#!V(PM$M*E_WV/9'-+(&G2OG2FD(EB'9VC<]/WR7F.+G*>M;2 3;G^'+U-
M<]'26)IP)J*KH,'2:6.8@> L34'0'*=3WCQ^UTRXJ!L-2P?)(!!LC*YXEK<T
MTJ#K&7$SXRWMK/?FXO2O,UWO=-"K<9!<\G,N4*>CBS2["N(P?QF(<9PF#9$%
M23[E0NVY7"]=&A@;\+6(0[%E+XF-36?)2$A(8!(>8L-T;5.7[K^\[?:N%8I-
MP\/4,JB[I([E&'H7D08AIH,P;6*S:7@(&RW+:E&OCFD+8[37*OJ3HCK6C]'/
MC>"5SE"*@I$_F\W210NQC >"HVD:SF..HF24!;G(YDS,,XY&:89F62I2EL;E
MDASTX>?O2$#JQU$.OSA2MM8V<J4#P? 0#6_4 JCC=9I-HN02L10,!TD(1C:J
MS^3LLY5>OMI+.9#/.(M&$2N7KOS)445.\!H8DLNB)!=!POA1#5V/(_ C3.5V
MD4 9GZ97X$L.38625( W/)3/*DZ89A.P,>73(9=F(A$%<?0U$%&:Y/"L]N5^
MF2@Q#@0*,FD^X3(\J>Y++3\,H";Z.T1=8F)]L.E#O?[(CZ[C .LO'BA]E+!X
M'O)F'"7S13,J2]H8;W>"9YI+[% ++UUS:)DFYRQP.&6.N[_K[C5:M+9E68:S
M))9%W ==#+-(GEAYCILJC0VVY9T))I>&01VZ).'08LQU V<X-#%U#GAWP-ZV
M8V#8)8]V;''7,8L:)EYR;E!F#CW##0@?4?,['5O<=8R"04<!U)XH'H:J)Z=2
M'[(L?IGQ$#KW/DL4OC;QL TM0RS'MA5P&>9MV#*<^V&+8%0W?@/7+PA<ZN3T
M43V[5C^ 0X-]O?H$..N:CHF(?E(,J(K.>!#'-V@2Q;'*IG2&BP:JO()Y"'&4
MI=,B;WX^\4<9A_*,("$CEDA:1[AQ)(TZA5$Y0!(%Y/HJC<(R\%*QLI4N5,TG
M4M&EH-$U/1=1_<2"7B:ZED]09[,EN)&RBM2IH<$G?W#6_]CW^[T:>O-ZX+_K
MG7WHG=80J:$/%Z>G1VV]:V&CL"4#U+1+J#5*YT))/)!T+4*1(0?I;-<RK&)P
M900$0PB&WJRB^HN,QSS(5:?!4Y0R$:NRY7"?@0H6#I8UK3;U+WJ78%<&($>G
M&#VOL JCJ2/X-)0=K5-F1CW5"DFYWUI6/I?2,H=K:?E<2M/KA&<@^_CVY-Q_
MW^]>G/9J>Q%N\12$^VY,/G MNQ>3Y04-+[%MFUYY07LLSIDVJI/?,/?KP5Q!
MQ _@W.)).'=BVV!^A451LCJOY:':AB(XSE4Y'NG_ ?;L0A0 D?RS_B*?M'5-
MFLG8&IQ4@O-_<A"O<OWO&C848@$>?=&UHD;^;"XJ]RFJ,PP:6L:AIQ+8J:U_
M _W;>+HXC*<0P'T[K/6D8X]UJW!E:\/;"'TWH[4B\:6MU5.49I&XJ0$V:Q*[
MY.17GJ5^)&IH LWELX"-N2]MQ,'P0%4DVJOD AE5_A#9C5^&<\EOA7,GE*,=
M5OA2\LTZCL%K?S 8]#_5-JYN_%,>M8M=(8L(?--V^[IDJB+%FLSE*)A&P+ (
M-EG97HG6D-%99VDE"GD9=&>WZ"#_AGB<<UAWJ()[J@>QMM:=)9M9EO/$EO34
M^S3HGWWTSS^_/^Z?5G:K"IHGA+A 7IIJ_'KO^.+\<_MGI+U+#*S(40V%P8S)
ME,(44=P--/P7D*]$OQ49 XB',<]J"LE*')67D72>"(EB0,2@3H'GB\+ [K-\
MP^6=71AX8C)/"%4^'Z;=0R]O#W/OC[U+'B#@!]XE,87W9)=X2\OS<,'"[J-?
M-@BJT]\D_.N1</'_@ULD?*AAGL+$A-IPI2[=+U)5G'^J+OW0>LA6W*IIE6KA
M]]%>?FZO5ZEK\]Y%-31/\N@R@81)8F'34!&-MIF&IKR$E%QNF2O1X."NBGJW
M/I5J"<9'=RBXO2:K(E*MJC!"8H;I0+A\(;A$X!\BTB(BM,NG.T2Z6K&'3]N;
9_^'"-)OD\VEGQ)B)+8_H_P,X$HUJ'A8     
 

Re: [PATCH] af_pppox: create module infrastructure for protocol modules

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20030428.222728.48508327.davem@redhat.com> (at Mon, 28 Apr 2003 22:27:28 -0700 (PDT)), "David S. Miller" <davem@redhat.com> says:

> So in the TCP/UDP/SCTP example case, a struct sock references the
> TCP/UDP/SCTP module _AND_ the ipv4/ipv6 module.

We probably need to refer IPv4 module as well as IPv6 module 
because of the IPv4-Mapped address feature for IPv6 sockets.

-- 
Hideaki YOSHIFUJI @ USAGI Project <yoshfuji@linux-ipv6.org>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA

Re: [PATCH] af_pppox: create module infrastructure for protocol modules

[Arnaldo Carvalho de Melo]

Em Mon, Apr 28, 2003 at 10:27:28PM -0700, David S. Miller escreveu:
>    From: Arnaldo Carvalho de Melo <acme@conectiva.com.br>
>    Date: Tue, 29 Apr 2003 03:12:27 -0300
> 
>    	Max, take a look and see if this same approach can be used in
>    bluetooth, I bet it can, its just a matter of not using struct
>    net_proto_family for bt_proto, just like pppox already was doing
>    before my changes :-)
> 
> Something similar can be done for ipv4/ipv6 by adding a struct module
> *owner member to struct inet_protosw etc. etc.

yes
 
> Although the idea is conceptually sound, you miss one crucial thing.
> Such struct sock's reference _TWO_ modules, the "PPPOE" module
> and the "PPPOX" module.

But what is the problem? at pppox_sk_alloc time I bump the PPPOE module refcnt,
making it safe, then it calls sk_alloc where it bumps the PPPOX module, making
it safe as well, so I'm taking care of both PPPOE and PPPOX.

> So in the TCP/UDP/SCTP example case, a struct sock references the
> TCP/UDP/SCTP module _AND_ the ipv4/ipv6 module.

ditto

> So what we'll need to do is use two owner pointers in struct sock,
> one for propagating the "struct socket" owner, and one for the
> "sub-protocol".
> 
> 	struct module	*owner;

This one is the net_families[net_family]->owner

> 	struct module	*sub_owner;

this one is the pppox_protos[protocol]->owner

I thought about it, but I don't see why the current scheme doesn't handle
it, care to elaborate a bit more? I don't doubt that I may be missing some
subtlety :-)

- Arnaldo

Re: [PATCH] af_pppox: create module infrastructure for protocol modules

[Arnaldo Carvalho de Melo]

Em Mon, Apr 28, 2003 at 11:00:45PM -0700, David S. Miller escreveu:
>    From: YOSHIFUJI Hideaki / ?$B5HF#1QL@ <yoshfuji@wide.ad.jp>
>    Date: Tue, 29 Apr 2003 15:46:51 +0900 (JST)
> 
>    We probably need to refer IPv4 module as well as IPv6 module 
>    because of the IPv4-Mapped address feature for IPv6 sockets.
>    
> Right.

This kind of subtlety is perfectly solved with the scheme now in DaveM's
tree for pppox, as the net family layer is where this kinds of special
cases are better handled.

I'll take a look at this and will try to come up with a patch.

- Arnaldo

Re: [PATCH] af_pppox: create module infrastructure for protocol modules

[Max Krasnyansky]

At 11:54 PM 4/28/2003, Arnaldo Carvalho de Melo wrote:
>Em Mon, Apr 28, 2003 at 10:27:28PM -0700, David S. Miller escreveu:
>>    From: Arnaldo Carvalho de Melo <acme@conectiva.com.br>
>>    Date: Tue, 29 Apr 2003 03:12:27 -0300
>> 
>>       Max, take a look and see if this same approach can be used in
>>    bluetooth, I bet it can, its just a matter of not using struct
>>    net_proto_family for bt_proto, just like pppox already was doing
>>    before my changes :-)
>> 
>> Something similar can be done for ipv4/ipv6 by adding a struct module
>> *owner member to struct inet_protosw etc. etc.
>
>yes
> 
>> Although the idea is conceptually sound, you miss one crucial thing.
>> Such struct sock's reference _TWO_ modules, the "PPPOE" module
>> and the "PPPOX" module.
This is not a problem. PPPOX (or ipv4/ipv6) module is not going to 
go away simply because PPPOE (or TCP/UDP) uses symbols from it.
There is no need to bump refcounters here. 
I think it's safe to say that protocols within the family always use 
symbols from main family module (they have to at list register/unregister).
So this is naturally taken care of.

>But what is the problem? at pppox_sk_alloc time I bump the PPPOE module refcnt,
>making it safe, then it calls sk_alloc where it bumps the PPPOX module, making
>it safe as well, so I'm taking care of both PPPOE and PPPOX.
Bumping refcount for PPPOX is an overhead without any benefits. Read above.

>>       struct module   *owner;
>
>This one is the net_families[net_family]->owner
>
>>       struct module   *sub_owner;
>
>this one is the pppox_protos[protocol]->owner
>
>I thought about it, but I don't see why the current scheme doesn't handle
>it, care to elaborate a bit more? I don't doubt that I may be missing some
>subtlety :-)
Ok. I'm going to ask this here again (to make sure that it's answered :))

- Why do we have to bump module refcount for 'struct sock' with _default_ callbacks ?
My answer is that we don't have to. I'd even say that we shouldn't. Module is not 
referenced from 'struct sock' in that case.

- Why are we not allowing net_family unregistration until all family sockets are gone ?
 From what I see it's only because current code does 'module_put(net_families[family]->owner)'.
But we don't care whether net_family is registered or not if we know who owns
the socket (i.e. sock->owner).

So far nobody gave me a clear answer to those questions. If we don't have to meet those
two restriction I don't see the point in creating all this net_family_get()/xxx_family_get(), 
etc, infrastructure. Patches and BK stuff that I've seen so far added more code (and a bit 
more overhead) than my original patch with sock->owner/sk->owner/sk_set_owner().
Yet they provide no additional flexibility or functionality.

I hate to repeat myself and please forgive me for that. But how is the current 
infrastructure better than following ?

----
sock_create()
{
        ...
        if (!try_module_get(npf->owner))
                goto fail;
        sock->owner = npf->owner;

        if (family_sock_create() < 0) {
                sock_release(sock);
                goto fail;
        }
        ...
}

 From this point on we don't care if net_proto_family is still registered or not, we know
who owns the socket.

sock_accept()
{
        ...
        __module_get(sock->owner);
        newsock->owner = sock->owner;
        ...
}

sock_release()
{
        ...
        module_put(sock->owner);
        ...
}

family_sock_create() (i.e. bt_sock_create(), etc)
{        
        ...
#ifdef SPECIAL_FAMILY_THAT_HANDLES_PROTOCOLS_IN_SEPARATE_MODULES
        if (!try_module_get(proto_module))
                goto fail;

        prev_owner = sock->owner;
        sock->owner = proto_module;
        module_put(prev_owner); 
#else
        /* Other families do not have to do anything */
#endif
        ...
}

That's it for 'struct socket'.  And I mean that is _it_. Family can be unregistered 
and stuff, we don't care, and why should we.

Now 'struct sock'. It can exist independently from 'struct socket' and therefor
is a separate issue.
We only care about callbacks (sk->data_ready(), etc) and private sk slab caches. Some 
protocols simply use default callback which belong to non modular part of the kernel. 
Those don't have to do anything. They will just work.
Other protocols replace callbacks and therefor have to make sure that module is still 
loaded while sk exists. For those modules we need the following

void sk_set_owner(struct module *owner)
{
        /* Module must already hold reference when it calls this function. */
        __module_get(owner);
        sk->owner = owner;
}
        
sk_free()
{
        ...
        module_put(sk->owner);
        ...
}

xxx_proto_set_sk_callbacks()
{
        ...
        sk_set_owner(THIS_MODULE);
        sk->destroy = proto_destroy;
        ...
}

That's it. There is no need to modify sk_alloc() or anything else. So it's not going
to introduce any overhead during socket creation from protocols that aren't modules or 
use default callback. And it gives us a flexibility of being able to pass ownership of 
the socket to another module or release module without having to access global family 
array.
For example if protocol wants to be unloaded but socket is still being used by net code
it could do something like
        write_lock(&sk->callback_lock);
        sk->data_ready = default_data_ready;
        ... other non default callbacks ...
        module_put(sk->owner);
        sk_set_owner(NULL);
        write_unlock(&sk->callback_lock);
---

Thanks
Max

Re: [PATCH] af_pppox: create module infrastructure for protocol modules

[Arnaldo Carvalho de Melo]

Em Tue, Apr 29, 2003 at 01:05:08PM -0700, Max Krasnyansky escreveu:
> At 11:54 PM 4/28/2003, Arnaldo Carvalho de Melo wrote:
> >Em Mon, Apr 28, 2003 at 10:27:28PM -0700, David S. Miller escreveu:
> >>    From: Arnaldo Carvalho de Melo <acme@conectiva.com.br>
> >>    Date: Tue, 29 Apr 2003 03:12:27 -0300

> >> Max, take a look and see if this same approach can be used in bluetooth, I
> >> bet it can, its just a matter of not using struct net_proto_family for
> >> bt_proto, just like pppox already was doing before my changes :-)

> >> Something similar can be done for ipv4/ipv6 by adding a struct module
> >> *owner member to struct inet_protosw etc. etc.

> >yes

> >> Although the idea is conceptually sound, you miss one crucial thing.
> >> Such struct sock's reference _TWO_ modules, the "PPPOE" module
> >> and the "PPPOX" module.

> This is not a problem. PPPOX (or ipv4/ipv6) module is not going to 
> go away simply because PPPOE (or TCP/UDP) uses symbols from it.
> There is no need to bump refcounters here. 

> I think it's safe to say that protocols within the family always use 
> symbols from main family module (they have to at list register/unregister).
> So this is naturally taken care of.

Because at the core level we don't know (and perhaps don't want to know) what
the specific net family module nor its protocol modules are doing
 
> >But what is the problem? at pppox_sk_alloc time I bump the PPPOE module refcnt,
> >making it safe, then it calls sk_alloc where it bumps the PPPOX module, making
> >it safe as well, so I'm taking care of both PPPOE and PPPOX.

> Bumping refcount for PPPOX is an overhead without any benefits. Read above.
 
> >>       struct module   *owner;

> >This one is the net_families[net_family]->owner

> >>       struct module   *sub_owner;

> >this one is the pppox_protos[protocol]->owner

> >I thought about it, but I don't see why the current scheme doesn't handle
> >it, care to elaborate a bit more? I don't doubt that I may be missing some
> >subtlety :-)

> Ok. I'm going to ask this here again (to make sure that it's answered :))

Hey, I answered already, but it seems you disagree with my view 8)
 
> - Why do we have to bump module refcount for 'struct sock' with _default_ callbacks ?
> My answer is that we don't have to. I'd even say that we shouldn't. Module is not 
> referenced from 'struct sock' in that case.

We just don't assume nothing about the net protocol family implementation, but now
that the infrastructure is in place we can surely study further optimizations that
don't break its layering abstraction.
 
> - Why are we not allowing net_family unregistration until all family sockets
> are gone?  From what I see it's only because current code does
> 'module_put(net_families[family]->owner)'. But we don't care whether
> net_family is registered or not if we know who owns the socket (i.e.
> sock->owner).

but we introduce aditional overhead in all struct sock created, why it is so important
to have the net_family unregistration done early rather than after all the struct sock
are freed?
 
> So far nobody gave me a clear answer to those questions. If we don't have to
> meet those two restriction I don't see the point in creating all this
> net_family_get()/xxx_family_get(), etc, infrastructure. Patches and BK stuff
> that I've seen so far added more code (and a bit more overhead) than my
> original patch with sock->owner/sk->owner/sk_set_owner().  Yet they provide
> no additional flexibility or functionality.

See, its a tradeoff, we don't bloat struct sock paying a bit of overhead in
other area.
 
> I hate to repeat myself and please forgive me for that. But how is the
> current infrastructure better than following ?

See my views above. And I'd love, as you, to have more people discussing this,
but most people I think that could help with this are damn busy with other
work, so at least we have a solid infrastructure in place and can revisit this
later if there is interest in further enhancing this.

- Arnaldo

Re: [PATCH] af_pppox: create module infrastructure for protocol modules

[Max Krasnyansky]

At 03:07 PM 4/29/2003, Arnaldo Carvalho de Melo wrote:
>Em Tue, Apr 29, 2003 at 01:05:08PM -0700, Max Krasnyansky escreveu:
>> >> Although the idea is conceptually sound, you miss one crucial thing.
>> >> Such struct sock's reference _TWO_ modules, the "PPPOE" module
>> >> and the "PPPOX" module.
>
>> This is not a problem. PPPOX (or ipv4/ipv6) module is not going to 
>> go away simply because PPPOE (or TCP/UDP) uses symbols from it.
>> There is no need to bump refcounters here. 
>
>> I think it's safe to say that protocols within the family always use 
>> symbols from main family module (they have to at list register/unregister).
>> So this is naturally taken care of.
>
>Because at the core level we don't know (and perhaps don't want to know) what
>the specific net family module nor its protocol modules are doing
We know that protocol has to at least register with the family. Which is enough
to keep family module loaded.

>> Ok. I'm going to ask this here again (to make sure that it's answered :))
>Hey, I answered already, but it seems you disagree with my view 8)
Of course I do. I wouldn't be arguing here if I didn't :).

>> - Why do we have to bump module refcount for 'struct sock' with _default_ callbacks ?
>> My answer is that we don't have to. I'd even say that we shouldn't. Module is not 
>> referenced from 'struct sock' in that case.
>
>We just don't assume nothing about the net protocol family implementation, but now
>that the infrastructure is in place we can surely study further optimizations that
>don't break its layering abstraction.
We already know which families change callbacks and which don't. I looked at that already.

>> - Why are we not allowing net_family unregistration until all family sockets
>> are gone?  From what I see it's only because current code does
>> 'module_put(net_families[family]->owner)'. But we don't care whether
>> net_family is registered or not if we know who owns the socket (i.e.
>> sock->owner).
>
>but we introduce aditional overhead in all struct sock created why it is so important
>to have the net_family unregistration done early rather than after all the struct sock
>are freed?
I'm not saying it's important. I'm saying we don't care, if we know who owns the socket.
And if we don't care we don't need net_family_get() and friends.

>> So far nobody gave me a clear answer to those questions. If we don't have to
>> meet those two restriction I don't see the point in creating all this
>> net_family_get()/xxx_family_get(), etc, infrastructure. Patches and BK stuff
>> that I've seen so far added more code (and a bit more overhead) than my
>> original patch with sock->owner/sk->owner/sk_set_owner().  Yet they provide
>> no additional flexibility or functionality.
>
>See, its a tradeoff, we don't bloat struct sock paying a bit of overhead in other area.
I wouldn't call one additional pointer a bloat (btw I mentioned that we have sk->prev and 
sk->pprev, we should be able to easily kill one of them).

>> I hate to repeat myself and please forgive me for that. But how is the
>> current infrastructure better than following ?
>
>See my views above. And I'd love, as you, to have more people discussing this,
>but most people I think that could help with this are damn busy with other
>work, so at least we have a solid infrastructure in place and can revisit this
>later if there is interest in further enhancing this.
That's the thing, I wouldn't call current infrastructure solid :)

Ok. Speaking about bloat. pppox mod refcounting. 
You added additional level if indirection to every 'struct socket' and 'struct sock'
allocation/dealocation.
For example to free pppoe sk we now have to call
        sk_free(sk)
                pppox_sk_free(sk)
                        pppox_protos[PPPOE]->sk_free(sk)
                module_put(pppox_protos[PPPOE]->owner)
instead of
        sk_free(sk)
                pppoe_sk_free(sk)
        module_put(sk->owner)

Same thing for pppox_release.
You also had to introduce new function pppox_sk_alloc() which is not needed otherwise.

Basically this entire patch 
        http://linux.bkbits.net:8080/linux-2.5/diffs/drivers/net/pppox.c@1.11?nav=index.html|ChangeSet@-2d|cset@1.1128.1.19

would have been

pppox.c
pppox_create()
{
        ...
+       if (!try_module_get(pppox_protos[protocol]->owner))
+               goto out; 
+
+       sock->owner = pppox_protos[protocol]->owner;
        ...
}

pppoe.c
pppoe_create()
{
        ...
+       sk->destruct = pppoe_sk_free;
+       sk_set_owner(THIS_MODULE);
        ...
}

IMO it's pretty clear which one has more bloat ;-)

Max

                


        

Re: [PATCH] af_pppox: create module infrastructure for protocol modules

[David S. Miller]

   From: Max Krasnyansky <maxk@qualcomm.com>
   Date: Tue, 29 Apr 2003 13:05:08 -0700

   - Why do we have to bump module refcount for 'struct sock' with
     _default_ callbacks ?

Nothing says that just because a sock uses default callbacks, it can't
be referenced in other ways by the implementation module, for example
it can sit in the protocol hash tables and that by itself requires
a module reference.

Re: [PATCH] af_pppox: create module infrastructure for protocol modules

[Max Krasnyansky]

On Tue, 2003-04-29 at 19:29, David S. Miller wrote:
>    From: Max Krasnyansky <maxk@qualcomm.com>
>    Date: Tue, 29 Apr 2003 13:05:08 -0700
> 
>    - Why do we have to bump module refcount for 'struct sock' with
>      _default_ callbacks ?
> 
> Nothing says that just because a sock uses default callbacks, it can't
> be referenced in other ways by the implementation module, for example
> it can sit in the protocol hash tables and that by itself requires
> a module reference.
That's a good point. However this is protocol's local business.
Netcore does not use those hashes directly. Netcore only uses things
like sk->state, sk->lock, etc and callbacks. So the callback is the only
reference, from netcore's point of view, into the protocol module.
Most protocols that I've looked at unlink sk from its hashes in
proto_sock_release(struct socket *sock) so it's enough to make sure that
struct socket is accounted for.  
And like I said before if protocol wants for some reason to be around
until sk is destroyed it will simply do sk_set_owner() right after
alloc_sk().

Max

Re: [PATCH] af_pppox: create module infrastructure for protocol modules

[David S. Miller]

   From: Max Krasnyansky <maxk@qualcomm.com>
   Date: 30 Apr 2003 11:11:37 -0700

   And like I said before if protocol wants for some reason to be
   around until sk is destroyed it will simply do sk_set_owner() right
   after alloc_sk().

Ok Maxim, thanks for your comments and insight.

I believe this whole discussion is going to take a different
course towards a solution, different than anything discussed
in this thread so far.

Please follow the discussion with topic "dev->destructor" on this list
(netdev) to see precisely what I am talking about.