Re: [BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family

Re: [BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family

[Max Krasnyansky]

At 12:26 PM 4/23/2003, Arnaldo Carvalho de Melo wrote:
>Em Wed, Apr 23, 2003 at 12:13:37PM -0700, Max Krasnyansky escreveu:
>> Hi Folks,
>> 
>> Can somebody (DaveM perhaps) please explain to me what the hell the following 
>> changset is doing in 2.5.68 
>>         http://linux.bkbits.net:8080/linux-2.5/cset@1.1118.1.1?nav=index.html|ChangeSet@-7d
>> ??
>> 
>> I've spent quite a bit of time looking into what's needed to fix socket module refcounting
>> issues and convicting DaveM and Alex that we need to fix them.
>> Here is the original thread
>>         http://marc.theaimsgroup.com/?l=linux-kernel&m=104308300808557&w=2
>
>I was not aware of this thread, will read it
Please do.

>> My patch was rejected without giving any technical explanation. I was waiting for Rusty's
>> __module_get() patch to get in, to release a new patch which addresses some issues that
>> came up during discussion.
>> Next thing you know, incomplete patch (changeset mentioned above) shows up in the BK and
>> 2.5.68. Without even a simple note on the lkm. And completely ignoring discussion that 
>> we had about this very issue just a few weeks ago.
>
>This is just the first part, DaveM already merged the second part, that deals with struct sock
That's exactly what surprised me. He rejected complete patch and accepted something incomplete
and broken.

>> I don't mind of course if some other (better) patch is accepted instead of mine. But patch 
>> that went in is incomplete and buggy. It doesn't handle accept case properly and doesn't 
>> address the issue of the 'struct sock' ownership (read original thread for more details).
>
>This is dealt with the following patch, and these patches were discussed on the netdev
>mailing list. Considerations ? I'll be sending today patches converting the net protocols
>that I maintain and then for some others that are ummaintained, etc.
I thought changes like that are always discussed on the lkml. They are directly related to the 
new module interface changes. It least CC lkml.

(I've subscribed to netdev list)

The latest patches that I've seen from you on netdev list and in main BK tree still don't 
handle accept() properly

net/socket.c:sys_accept()
        ...
        if (!(newsock = sock_alloc())) 
                goto out_put;

        newsock->type = sock->type;
        newsock->ops = sock->ops;
         ...

newsock is not accounted for (sock_alloc() doesn't bump module refcount).

> struct sock *sk_alloc(int family, int priority, int zero_it, kmem_cache_t *slab)
> {
>-       struct sock *sk;
>-       
>+       struct sock *sk = NULL;
>+
>+       if (!net_family_get(family))
>+               goto out;

Ok. This is wrong. Which should be clear from reading the thread that I mentioned.
Owner of the net_proto_family is not necessarily the owner of the 'struct sock'.
Example: af_inet module registers net_proto_family but udp module owns the socket.
(not that IPv4 code is modular but just an example). Another example would be Bluetooth.
We have Bluetooth core that registers Bluetooth proto_family and several modules
that handle specific protocols (l2cap, rfcomm, etc).

Also net_proto_family has pretty much nothing to do with the struct sock. The only reason 
we would want to hold reference to the module is if the module has replaced default 
callbacks (i.e. sk->data_ready(), etc).
So my point is we need sk->owner field. 

I'd also prefer to see sock->owner which gives more flexibility (i.e. net_proto_family can 
be unregistered while struct socket still exist, etc). 
net_family_get/put() makes no sense net_proto_family has only one function npf->create() 
which is referenced only from net/socket.c. struct socket should inherit owner field from
struct net_proto_family by default but protocol should be able to assign ownership to a 
different module if it needs to.

So I'd suggest reverting the patches that went in and applying my patch instead. Not because 
its mine but because IMO it's a better starting point. Dave, comments ?

Thanks
Max

Re: [BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family

[David S. Miller]

   From: Max Krasnyansky <maxk@qualcomm.com>
   Date: Wed, 23 Apr 2003 15:51:11 -0700

   >This is just the first part, DaveM already merged the second part,
   >that deals with struct sock 

   That's exactly what surprised me. He rejected complete patch and
   accepted something incomplete and broken.

No, it was not broken, because he told me completely where he
was going with his changes.  He was building infrastructure piece by
piece, and that's always an acceptable way to do things as long
as it is explained where one is going with the changes.

Your stuff was unacceptable from the start because you didn't put
the ->owner into the protocol ops.
   
   I thought changes like that are always discussed on the lkml.

No, networking patches can go solely to netdev or linux-net.
   

Re: [BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family

[Max Krasnyansky]

At 04:30 PM 4/23/2003, David S. Miller wrote:
>   From: Max Krasnyansky <maxk@qualcomm.com>
>   Date: Wed, 23 Apr 2003 15:51:11 -0700
>
>   >This is just the first part, DaveM already merged the second part,
>   >that deals with struct sock 
>
>   That's exactly what surprised me. He rejected complete patch and
>   accepted something incomplete and broken.
>
>No, it was not broken, because he told me completely where he
>was going with his changes. 
Of course it was and still is. New socket is allocated without incrementing 
modules ref count in sys_accept().

>He was building infrastructure piece by piece, and that's always an acceptable 
>way to do things as long as it is explained where one is going with the changes.
Oh, I see. And I just sent a patch without any explanation. Ok. 
(you might want to reread our original discussion again).

>Your stuff was unacceptable from the start because you didn't put
>the ->owner into the protocol ops.
But you didn't tell me that. You just said that it's "an ugly hack" without
giving any other feedback.
 
->owner field in protocol ops did come up during discussion (I think Rusty brought 
that up) and I explained why it shouldn't be there. But again there was no feed back 
from you. You just ignored that thread at some point. 

btw I still don't see ->owner in protocol ops. I read archives of netdev. You guys 
didn't even talk about that.

Anyway it's not important who said what now. You chose to ignore stuff that I did, fine.
What about this though

>>struct sock *sk_alloc(int family, int priority, int zero_it, kmem_cache_t *slab)
>>{
>>- struct sock *sk;
>>- 
>>+ struct sock *sk = NULL;
>>+
>>+ if (!net_family_get(family))
>>+ goto out;
>Ok. This is wrong. Which should be clear from reading the thread that I mentioned.
>Owner of the net_proto_family is not necessarily the owner of the 'struct sock'.
>Example: af_inet module registers net_proto_family but udp module owns the socket.
>(not that IPv4 code is modular but just an example). Another example would be Bluetooth.
>We have Bluetooth core that registers Bluetooth proto_family and several modules
>that handle specific protocols (l2cap, rfcomm, etc).
>
>Also net_proto_family has pretty much nothing to do with the struct sock. The only reason 
>we would want to hold reference to the module is if the module has replaced default 
>callbacks (i.e. sk->data_ready(), etc).
>So my point is we need sk->owner field. 
>
>I'd also prefer to see sock->owner which gives more flexibility (i.e. net_proto_family can 
>be unregistered while struct socket still exist, etc). 
>net_family_get/put() makes no sense net_proto_family has only one function npf->create() 
>which is referenced only from net/socket.c. struct socket should inherit owner field from
>struct net_proto_family by default but protocol should be able to assign ownership to a 
>different module if it needs to.

Max

Re: [BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family

[David S. Miller]

   From: Max Krasnyansky <maxk@qualcomm.com>
   Date: Wed, 23 Apr 2003 18:41:56 -0700

   At 04:30 PM 4/23/2003, David S. Miller wrote:
   >Your stuff was unacceptable from the start because you didn't put
   >the ->owner into the protocol ops.
   But you didn't tell me that. You just said that it's "an ugly hack" without
   giving any other feedback.
    
As you mention, Rusty said this.

   What about this though
   
I'm sure Arnaldo will deal with the sys_accept() issues.
But this is a minor issue, Arnaldo's stuff is architectually
solid.

Re: [BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family

[Max Krasnyansky]

At 08:29 PM 4/23/2003, David S. Miller wrote:
>   From: Max Krasnyansky <maxk@qualcomm.com>
>   Date: Wed, 23 Apr 2003 18:41:56 -0700
>
>   At 04:30 PM 4/23/2003, David S. Miller wrote:
>   >Your stuff was unacceptable from the start because you didn't put
>   >the ->owner into the protocol ops.
>   But you didn't tell me that. You just said that it's "an ugly hack" without
>   giving any other feedback.
>    
>As you mention, Rusty said this.
>
>   What about this though
>   
>I'm sure Arnaldo will deal with the sys_accept() issues.
>But this is a minor issue, Arnaldo's stuff is architectually
>solid.

:) 

Ok Dave. I'm not sure why you're _completely_ ignoring my arguments.
You should have just said from the beginning that you were going to
ignore what I have to say regarding that issue. At least I wouldn't 
have wasted my time.

Sorry for bugging you.
Max

Re: [BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family

[Arnaldo Carvalho de Melo]

Em Wed, Apr 23, 2003 at 03:51:11PM -0700, Max Krasnyansky escreveu:
> At 12:26 PM 4/23/2003, Arnaldo Carvalho de Melo wrote:
> >Em Wed, Apr 23, 2003 at 12:13:37PM -0700, Max Krasnyansky escreveu:
> >> Hi Folks,
> >> 
> >> Can somebody (DaveM perhaps) please explain to me what the hell the following 
> >> changset is doing in 2.5.68 
> >>         http://linux.bkbits.net:8080/linux-2.5/cset@1.1118.1.1?nav=index.html|ChangeSet@-7d
> >> ??
> >> 
> >> I've spent quite a bit of time looking into what's needed to fix socket module refcounting
> >> issues and convicting DaveM and Alex that we need to fix them.
> >> Here is the original thread
> >>         http://marc.theaimsgroup.com/?l=linux-kernel&m=104308300808557&w=2
> >
> >I was not aware of this thread, will read it
> Please do.

I did, there are several valid points, I forgot the sys_accept case, I'm
studying the code to see how to properly fix it, but one thing I want covered
is keeping the net_families registered while there is still a struct sock
associated with the proto family, while not allowing the criation of new sockets
till the last struct sock is sk_freed.

Think about protecting the module with the top level network protocol family
->create code on sock_create level and the modules with specific protocols at
<net_proto_family>_sock_create level.
 
> >> I don't mind of course if some other (better) patch is accepted instead of mine. But patch 
> >> that went in is incomplete and buggy. It doesn't handle accept case properly and doesn't 
> >> address the issue of the 'struct sock' ownership (read original thread for more details).
> >
> >This is dealt with the following patch, and these patches were discussed on the netdev
> >mailing list. Considerations ? I'll be sending today patches converting the net protocols
> >that I maintain and then for some others that are ummaintained, etc.
> I thought changes like that are always discussed on the lkml. They are directly related to the 
> new module interface changes. It least CC lkml.
> 
> (I've subscribed to netdev list)
> 
> The latest patches that I've seen from you on netdev list and in main BK tree still don't 
> handle accept() properly
> 
> net/socket.c:sys_accept()
>         ...
>         if (!(newsock = sock_alloc())) 
>                 goto out_put;
> 
>         newsock->type = sock->type;
>         newsock->ops = sock->ops;
>          ...
> 
> newsock is not accounted for (sock_alloc() doesn't bump module refcount).
> 
> > struct sock *sk_alloc(int family, int priority, int zero_it, kmem_cache_t *slab)
> > {
> >-       struct sock *sk;
> >-       
> >+       struct sock *sk = NULL;
> >+
> >+       if (!net_family_get(family))
> >+               goto out;
> 
> Ok. This is wrong. Which should be clear from reading the thread that I mentioned.
> Owner of the net_proto_family is not necessarily the owner of the 'struct sock'.
> Example: af_inet module registers net_proto_family but udp module owns the socket.
> (not that IPv4 code is modular but just an example). Another example would be Bluetooth.
> We have Bluetooth core that registers Bluetooth proto_family and several modules
> that handle specific protocols (l2cap, rfcomm, etc).

I'm think that this can be handled at this protocol family code level, i.e.  if
the net_proto_family wants to further modularise based on specific protocol
type this has to be refcounted at this level, example, at bt_sock_create.
 
> Also net_proto_family has pretty much nothing to do with the struct sock. The only reason 
> we would want to hold reference to the module is if the module has replaced default 
> callbacks (i.e. sk->data_ready(), etc).
> So my point is we need sk->owner field. 

That is a way to solve it, yes, but I don't think the only one,
net_proto_family has to do with the struct sock as those data structures are
related to the network protocol family.

> I'd also prefer to see sock->owner which gives more flexibility (i.e.
> net_proto_family can be unregistered while struct socket still exist, etc).
> net_family_get/put() makes no sense net_proto_family has only one function
> npf->create() which is referenced only from net/socket.c. struct socket
> should inherit owner field from struct net_proto_family by default but
> protocol should be able to assign ownership to a different module if it needs
> to.

I'm studying how to signal that the net_proto_family was unregistered while
still having the entry in net_families available, that is a problem with the
current infrastructure in Linus tree.

Sorry, today I didn't had time to work on this at night at home, but tomorrow
I'll be working on this, thing is, as I'm working only on having the
infrastructure in place and no protocols have ->owner set there is no way to
trigger the problems discussed in this message in any way.

Ah, and thanks for raising these issues and pointing me to the previous
discussion that I had not read before, just too much e-mail on lkml, netdev
is more manageable :-)

- Arnaldo

Re: [BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family

[Max Krasnyansky]

Hi Arnaldo,

>I did, there are several valid points, I forgot the sys_accept case, I'm
>studying the code to see how to properly fix it,
It's pretty easy if we have sock->owner. Take a look how my patch does it 
(try_module_get should be replaced with __module_get()).
It also covers the case when net_family_owner transferred ownership to the
other module.

>but one thing I want covered
>is keeping the net_families registered while there is still a struct sock
>associated with the proto family, while not allowing the criation of new sockets
>till the last struct sock is sk_freed.
Why ?
Like I said all we care about is if non default callbacks in struct sock are valid.
Please read below.

>Think about protecting the module with the top level network protocol family
>->create code on sock_create level and the modules with specific protocols at
><net_proto_family>_sock_create level.
With the current infrastructure you'd need some kind of callback from sk_free() for
that to work.
I think it's simpler with sk->owner.

>> > struct sock *sk_alloc(int family, int priority, int zero_it, kmem_cache_t *slab)
>> > {
>> >-       struct sock *sk;
>> >-       
>> >+       struct sock *sk = NULL;
>> >+
>> >+       if (!net_family_get(family))
>> >+               goto out;
>> 
>> Ok. This is wrong. Which should be clear from reading the thread that I mentioned.
>> Owner of the net_proto_family is not necessarily the owner of the 'struct sock'.
>> Example: af_inet module registers net_proto_family but udp module owns the socket.
>> (not that IPv4 code is modular but just an example). Another example would be Bluetooth.
>> We have Bluetooth core that registers Bluetooth proto_family and several modules
>> that handle specific protocols (l2cap, rfcomm, etc).
>
>I'm think that this can be handled at this protocol family code level, i.e.  if
>the net_proto_family wants to further modularise based on specific protocol
>type this has to be refcounted at this level, example, at bt_sock_create.
Sure. Transfer of the ownership has to be done on that level. What I'm saying is
that everything else can be done in generic code. 

>> Also net_proto_family has pretty much nothing to do with the struct sock. The only reason 
>> we would want to hold reference to the module is if the module has replaced default 
>> callbacks (i.e. sk->data_ready(), etc).
>> So my point is we need sk->owner field. 
>
>That is a way to solve it, yes, but I don't think the only one,
>net_proto_family has to do with the struct sock as those data structures are
>related to the network protocol family.
By the same logic all things are related in the kernel :).
struct socket uses net_proto_family ops and stuff and therefor is related.
But struct sock is just a callbacks.

>> I'd also prefer to see sock->owner which gives more flexibility (i.e.
>> net_proto_family can be unregistered while struct socket still exist, etc).
>> net_family_get/put() makes no sense net_proto_family has only one function
>> npf->create() which is referenced only from net/socket.c. struct socket
>> should inherit owner field from struct net_proto_family by default but
>> protocol should be able to assign ownership to a different module if it needs
>> to.
>
>I'm studying how to signal that the net_proto_family was unregistered while
>still having the entry in net_families available, that is a problem with the
>current infrastructure in Linus tree.
Why do you need that ? 
Let's just get rid of the net_family_get/put thing. Each socket will have owner
field so we know who owns it and there is no need to access global net_family 
table.

>Sorry, today I didn't had time to work on this at night at home, but tomorrow
>I'll be working on this, thing is, as I'm working only on having the
>infrastructure in place and no protocols have ->owner set there is no way to
>trigger the problems discussed in this message in any way.
I actually had it all working with the patch that I mentioned. I can go ahead 
and add support to Bluetooth stuff, that's the reason I started that effort
in first place. However current code in main BK tree doesn't cover scenario  
when owner of net_proto_family != owner of the socket.

Here is how I think it should work.

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 call this function. */
        __module_get(owner);
        sk->owner = owner;
}
        
sk_free()
        ...
        module_put(sk->owner);
        ...

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 by 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);

Here is my original explanation.

>>>Here is the list of sk callbacks
>>>        void                    (*state_change)(struct sock *sk);
>>>        void                    (*data_ready)(struct sock *sk, int bytes);
>>>        void                    (*write_space)(struct sock *sk);
>>>        void                    (*error_report)(struct sock *sk);
>>>        int                     (*backlog_rcv) (struct sock *sk, struct sk_buff *skb);  
>>>        void                    (*destruct)(struct sock *sk);
>>>Net core doesn't make any other calls into the protocol module.
>>>
>>>In any case if protocol module wants, for some reason, to be around until sk is destroyed
>>>it can call sk_set_owner() right after sk_alloc(), even if it uses default callbacks.
>>>
>>>While looking at the net families that we support I realized that there is one more reason
>>>why protocol may have to call sk_set_owner(). Is if protocol uses private slab cache for its 
>>>sks. Otherwise cache will be destroyed during module unloading but sk may still be alive.
>>>
>>>Here is a little summary about families that I looked at:
>>>        Netlink - Global sk cache, replaces sk->destruct() callback.
>>>                  Must call sk_set_owner()
>>>
>>>        Unix -  Private sk cache, replaces sk->destruct() and sk->write_space() callbacks
>>>                Must call sk_set_owner()
>>>
>>>        IPv4/6 - Private cache, non-default callbacks.
>>>                Must call sk_set_owner() (cannot be a module, so we don't really care)
>>>
>>>        IPX - Global cache, default callbacks.
>>>
>>>        ATM - Global cache, default callbacks.
>>>                
>>>        IRDA - Global cache, default callbacks.
>>>
>>>        Bluetooth - Private cache (don't really need it, will change to global), non-default callbacks
>>>                Must call sk_set_owner()
>>>
>>>        AX25 - Global cache, non-default sk->destroy()
>>>                Must call sk_set_owner()
>>>
>>>        Rose - Global cache, default callbacks. 
>>>        
>>>        DecNet - Private cache, non-default callbacks.
>>>                Must call sk_set_owner()
>>>        
>>>        LLC - Global cache, default callbacks.

And here is the patch that implements it
        http://marc.theaimsgroup.com/?l=linux-kernel&m=104308300808557&w=2
(try_module_get() should be replaced with __module_get() in some places).

>Ah, and thanks for raising these issues and pointing me to the previous
>discussion that I had not read before,
>just too much e-mail on lkml, netdev is more manageable :-)
You're very welcome. I'm very interested to get this thing fixed.

(I wish Dave and Alexey were involved in this discussion and expressed their
thoughts/ideas/concerns instead of ignoring it.)

Max

Re: [BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family

[Arnaldo Carvalho de Melo]

Em Thu, Apr 24, 2003 at 12:33:35PM -0700, Max Krasnyansky escreveu:
> Hi Arnaldo,
> 
> >I did, there are several valid points, I forgot the sys_accept case, I'm
> >studying the code to see how to properly fix it,

> It's pretty easy if we have sock->owner. Take a look how my patch does it 
> (try_module_get should be replaced with __module_get()).
> It also covers the case when net_family_owner transferred ownership to the
> other module.

Yes, this gives us more flexibility, but as well some more overhead in the
common struct sock, I have worked in 2.5 to reduce its size, and want to
reduce it further, if possible. But read on, other reasons below.
 
> >but one thing I want covered
> >is keeping the net_families registered while there is still a struct sock
> >associated with the proto family, while not allowing the criation of new sockets
> >till the last struct sock is sk_freed.
> Why ?
> Like I said all we care about is if non default callbacks in struct sock are valid.
> Please read below.
> 
> >Think about protecting the module with the top level network protocol family
> >->create code on sock_create level and the modules with specific protocols at
> ><net_proto_family>_sock_create level.

> With the current infrastructure you'd need some kind of callback from
> sk_free() for that to work.  I think it's simpler with sk->owner.

not a callback, it'd be in sk_free itself (talking from the top of my head,
just arrived home and will be further studying this now), I pretty much want to
have protocol family maintainers not worrying about module refcounting at all
in the default case of no further modularisation per specific prococol, that
would save, for instance, Steven Whitehouse some work on DecNET :)
 
> >> > struct sock *sk_alloc(int family, int priority, int zero_it, kmem_cache_t *slab)
> >> > {
> >> >-       struct sock *sk;
> >> >-       
> >> >+       struct sock *sk = NULL;
> >> >+
> >> >+       if (!net_family_get(family))
> >> >+               goto out;

> >> Ok. This is wrong. Which should be clear from reading the thread that I
> >> mentioned.  Owner of the net_proto_family is not necessarily the owner of
> >> the 'struct sock'.  Example: af_inet module registers net_proto_family but
> >> udp module owns the socket.  (not that IPv4 code is modular but just an
> >> example). Another example would be Bluetooth.  We have Bluetooth core that
> >> registers Bluetooth proto_family and several modules that handle specific
> >> protocols (l2cap, rfcomm, etc).

> >I'm think that this can be handled at this protocol family code level, i.e.
> >if the net_proto_family wants to further modularise based on specific
> >protocol type this has to be refcounted at this level, example, at
> >bt_sock_create.

> Sure. Transfer of the ownership has to be done on that level. What I'm saying
> is that everything else can be done in generic code. 

So we're in agreement on having most of this infrastructure in generic code :)
 
> >> Also net_proto_family has pretty much nothing to do with the struct sock.
> >> The only reason we would want to hold reference to the module is if the
> >> module has replaced default callbacks (i.e. sk->data_ready(), etc).  So my
> >> point is we need sk->owner field. 

> >That is a way to solve it, yes, but I don't think the only one,
> >net_proto_family has to do with the struct sock as those data structures are
> >related to the network protocol family.

> By the same logic all things are related in the kernel :).

:-)

> struct socket uses net_proto_family ops and stuff and therefor is related.
> But struct sock is just a callbacks.

But I want to avoid having another thing to worry about for net protocol
family writers, i.e. not having to add a sk_set_owner if one decides it has
to change one of the default callbacks.
 
> >> I'd also prefer to see sock->owner which gives more flexibility (i.e.
> >> net_proto_family can be unregistered while struct socket still exist,
> >> etc).  net_family_get/put() makes no sense net_proto_family has only one
> >> function npf->create() which is referenced only from net/socket.c. struct
> >> socket should inherit owner field from struct net_proto_family by default
> >> but protocol should be able to assign ownership to a different module if
> >> it needs to.

> >I'm studying how to signal that the net_proto_family was unregistered while
> >still having the entry in net_families available, that is a problem with the
> >current infrastructure in Linus tree.

> Why do you need that ? 

> Let's just get rid of the net_family_get/put thing. Each socket will have
> owner field so we know who owns it and there is no need to access global
> net_family table.

I'm trying to avoid adding a pointer to struct sock, if it is not possible
at all to avoid, ok, we do it, but I think we can have this at just one
place, the net_family table.

> >Sorry, today I didn't had time to work on this at night at home, but
> >tomorrow I'll be working on this, thing is, as I'm working only on having
> >the infrastructure in place and no protocols have ->owner set there is no
> >way to trigger the problems discussed in this message in any way.

> I actually had it all working with the patch that I mentioned. I can go ahead 
> and add support to Bluetooth stuff, that's the reason I started that effort
> in first place. However current code in main BK tree doesn't cover scenario  
> when owner of net_proto_family != owner of the socket.

> Here is how I think it should work.
> 
> 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 call this function. */
>         __module_get(owner);
>         sk->owner = owner;
> }

This is the additional rule I'm trying to avoid.
         
> sk_free()
>         ...
>         module_put(sk->owner);
>         ...

this is already in BK tree.
 
> >>>        IPv4/6 - Private cache, non-default callbacks.
> >>>                Must call sk_set_owner() (cannot be a module, so we don't really care)

Well, I care, I wish to have this as a module at some point, if allowed by
DaveM, of course 8)

- Arnaldo

Re: [BK ChangeSet@1.1118.1.1] new module infrastructure for net_proto_family

[Max Krasnyansky]

At 04:02 PM 4/24/2003, Arnaldo Carvalho de Melo wrote:
>Em Thu, Apr 24, 2003 at 12:33:35PM -0700, Max Krasnyansky escreveu:
>> Hi Arnaldo,
>> 
>> >I did, there are several valid points, I forgot the sys_accept case, I'm
>> >studying the code to see how to properly fix it,
>
>> It's pretty easy if we have sock->owner. Take a look how my patch does it 
>> (try_module_get should be replaced with __module_get()).
>> It also covers the case when net_family_owner transferred ownership to the
>> other module.
>
>Yes, this gives us more flexibility, but as well some more overhead in the
>common struct sock, I have worked in 2.5 to reduce its size, and want to
>reduce it further, if possible. 
Easy.
struct sock {
        ...
        struct sock             *next;
        struct sock             **pprev;
        struct sock             *bind_next;
        struct sock             **bind_pprev;
        struct sock             *prev;

At least one of the prev can go.

>> >Think about protecting the module with the top level network protocol family
>> >->create code on sock_create level and the modules with specific protocols at
>> ><net_proto_family>_sock_create level.
>
>> With the current infrastructure you'd need some kind of callback from
>> sk_free() for that to work.  I think it's simpler with sk->owner.
>
>not a callback, it'd be in sk_free itself (talking from the top of my head,
>just arrived home and will be further studying this now), I pretty much want to
>have protocol family maintainers not worrying about module refcounting at all
>in the default case of no further modularisation per specific prococol, that
>would save, for instance, Steven Whitehouse some work on DecNET :)

sk_free() doesn't know which module owns the socket in 
"net_proto_family owner != socket owner" case. 

BTW Let's call that case "NOnSO" I'm referring to it way too often :)

>> Sure. Transfer of the ownership has to be done on that level. What I'm saying
>> is that everything else can be done in generic code. 
>
>So we're in agreement on having most of this infrastructure in generic code :)
I guess so :). But haven't seen a generic solution for the NOnSO from you yet ;-). 

>> struct socket uses net_proto_family ops and stuff and therefor is related.
>> But struct sock is just a callbacks.
>
>But I want to avoid having another thing to worry about for net protocol
>family writers, i.e. not having to add a sk_set_owner if one decides it has
>to change one of the default callbacks.
Well, that means that refcounting overhead will affect even the protocols that 
don't have to keep track of 'struct sock'. It also means that those protocols 
will not unloadable until all sk's are killed. Even though sk's have no references 
whatsoever to the module that created them. 

>> Let's just get rid of the net_family_get/put thing. Each socket will have
>> owner field so we know who owns it and there is no need to access global
>> net_family table.
>
>I'm trying to avoid adding a pointer to struct sock, if it is not possible
>at all to avoid, ok, we do it, but I think we can have this at just one
>place, the net_family table.
Killing one of the list pointers from 'struct sock' shouldn't be a problem (see above). 

One net_family table won't handle NOnSO. Even if it will, protocol writers will have to 
worry about proper initialization of the tables anyway. Especially if someone comes up 
with something like:
        Family X (module X.ko)
                Proto X1 (module X1.ko)
                        Subproto X1-A (module X1-A.ko)
not that it makes a lot of sense right now but why restrict it if we don't have to. 

>> void sk_set_owner(struct module *owner)
>> {
>>         /* Module must already hold reference when it call this function. */
>>         __module_get(owner);
>>         sk->owner = owner;
>> }
>
>This is the additional rule I'm trying to avoid.

It's not a big deal rule. All protocols do hold reference when they allocate sk.

>> sk_free()
>>         ...
>>         module_put(sk->owner);
>>         ...
>
>this is already in BK tree.
No. I mean NOnSO ;-). 
What's in BK is net_family_put(family) which is by no means the same.

>> >>>        IPv4/6 - Private cache, non-default callbacks.
>> >>>                Must call sk_set_owner() (cannot be a module, so we don't really care)
>
>Well, I care, I wish to have this as a module at some point, if allowed by
>DaveM, of course 8)
If I remember correctly DaveM was one of those are strongly against that.

Max

Re: New module infrastructure for net_proto_family

[Max Krasnyansky]

Hi Arnaldo,

Hmm, no comments on my last email (http://marc.theaimsgroup.com/?l=linux-netdev&m=105123134301565&w=2) 
Are you trying to ignore me too ? ;-)

Anyway, here is another idea. How about this (untested, uncompiled, just rfc).

--- 1.14/include/linux/net.h    Tue Apr 22 22:48:58 2003
+++ edited/include/linux/net.h  Mon Apr 28 13:59:16 2003
@@ -81,6 +81,7 @@
        struct sock             *sk;
        wait_queue_head_t       wait;
        short                   type;
+       short                   protocol;
        unsigned char           passcred;
 };
 
@@ -132,6 +133,10 @@
 struct net_proto_family {
        int             family;
        int             (*create)(struct socket *sock, int protocol);
+
+       int             (*get)(int protocol);
+       void            (*put)(int protocol);
+
        /* These are counters for the number of different methods of
           each we support */
        short           authentication;
@@ -140,8 +145,8 @@
        struct module   *owner;
 };
 
-extern int          net_family_get(int family);
-extern void         net_family_put(int family);
+extern int          net_family_get(int family, int protocol);
+extern void         net_family_put(int family, int protocol);
 
 struct iovec;

--- 1.53/net/socket.c   Tue Apr 22 23:13:31 2003
+++ edited/net/socket.c Mon Apr 28 13:55:30 2003
@@ -147,27 +147,31 @@
        BUG();
 }
 
-int net_family_get(int family)
+int net_family_get(int family, int protocol)
 {
        struct net_proto_family *prot = net_families[family];
        int rc = 1;
 
        barrier();
-       if (likely(prot != NULL))
+       if (likely(prot != NULL)) {
                rc = try_module_get(prot->owner);
-       else
+               if (!rc && prot->get && (rc = prot->get(protocol)) != 0)
+                       module_put(prot->owner);
+       } else
                net_family_bug(family);
        return rc;
 }
 
-void net_family_put(int family)
+void net_family_put(int family, int protocol)
 {
        struct net_proto_family *prot = net_families[family];
 
        barrier();
-       if (likely(prot != NULL))
+       if (likely(prot != NULL)) {
                module_put(prot->owner);
-       else
+               if (prot->put)
+                       prot->put(protocol);
+       } else
                net_family_bug(family);
 }
 
@@ -539,7 +543,7 @@
 
                sock->ops->release(sock);
                sock->ops = NULL;
-               net_family_put(family);
+               net_family_put(family, sock->protocol);
        }
 
        if (sock->fasync_list)
@@ -1089,10 +1093,11 @@
                goto out;
        }
 
-       sock->type  = type;
+       sock->type     = type;
+       sock->protocol = protocol;
 
        i = -EBUSY;
-       if (!net_family_get(family))
+       if (!net_family_get(family, protocol))
                goto out_release;
 
        if ((i = net_families[family]->create(sock, protocol)) < 0) 
@@ -1910,6 +1915,7 @@
                net_families[ops->family]=ops;
                err = 0;
        }
+
        net_family_write_unlock();
        return err;
 }

-----

So families that handle protos in separate modules would have their own 
family->put/get() methods that net core calls.

I still think that sock->owner is a better (more flexible) solution though.

Max

Re: New module infrastructure for net_proto_family

[Arnaldo Carvalho de Melo]

Em Mon, Apr 28, 2003 at 02:10:11PM -0700, Max Krasnyansky escreveu:
> Hi Arnaldo,
> 
> Hmm, no comments on my last email (http://marc.theaimsgroup.com/?l=linux-netdev&m=105123134301565&w=2) 
> Are you trying to ignore me too ? ;-)

Nope, I was working on the higher layer first, to then come to the net families
that are already modular and have per-protocol modules, and I'm starting to look
into the only, AFAIK, network family that has this in place: bluetooth :-)

Having said that I haven't fully studied this I see two scenarios (brainstorming):

1. the net family that wants per protocol "sub" modules "duplicates" the
infrastructure having PROTO_sk_alloc and PROTO_destruct (the sk_free
sk->destruct hook call), PROTO_sk_alloc uses its net_families equivalent
(bt_proto in bluetooth) to find the owner (the "sub" module, i.e. per protocol
module) and PROTO_net_family_gets it, then calls sk_alloc proper, and when the
last reference to the sock is released the sk->destruct is called
(PROTO_destruct) does the PROTO_net_family_put. Ditto for the socket case,
where PROTO_create, before calling the ->create of the "sub" module does the
PROTO_net_family_get, and at release time its PROTO_release does the same thing
that sock_release does. Something like this may well need extra info to be kept
at the private area of the proto family in struct sock protinfo or private slab
cache.

That is, we have a higher layer for net families, with locking for the whole
family done like it is on the tree now and a lower layer at the specific
net family, both having the same behaviour at its layers.

This option seems to be easy to implement with the current bluetooth
infrastructure (i.e. it has a net_families equivalent, it does the switching at
bt_create time, etc).

2. use the sk->prot (struct proto) infrastructure in some way.

Comments?
 
> Anyway, here is another idea. How about this (untested, uncompiled, just rfc).

I didn't liked it, kind of layering violation that I'm trying to avoid.

- Arnaldo

Re: New module infrastructure for net_proto_family

[Max Krasnyansky]

At 06:43 PM 4/28/2003, Arnaldo Carvalho de Melo wrote:
>Em Mon, Apr 28, 2003 at 02:10:11PM -0700, Max Krasnyansky escreveu:
>> Hi Arnaldo,
>> 
>> Hmm, no comments on my last email (http://marc.theaimsgroup.com/?l=linux-netdev&m=105123134301565&w=2) 
>> Are you trying to ignore me too ? ;-)
>
>Nope, I was working on the higher layer first, to then come to the net families
>that are already modular and have per-protocol modules, and I'm starting to look
>into the only, AFAIK, network family that has this in place: bluetooth :-)
>
>Having said that I haven't fully studied this I see two scenarios (brainstorming):
>
>1. the net family that wants per protocol "sub" modules "duplicates" the
>infrastructure having PROTO_sk_alloc and PROTO_destruct (the sk_free
>sk->destruct hook call), PROTO_sk_alloc uses its net_families equivalent
>(bt_proto in bluetooth) to find the owner (the "sub" module, i.e. per protocol
>module) and PROTO_net_family_gets it, then calls sk_alloc proper, and when the
>last reference to the sock is released the sk->destruct is called
>(PROTO_destruct) does the PROTO_net_family_put. Ditto for the socket case,
>where PROTO_create, before calling the ->create of the "sub" module does the
>PROTO_net_family_get, and at release time its PROTO_release does the same thing
>that sock_release does. Something like this may well need extra info to be kept
>at the private area of the proto family in struct sock protinfo or private slab
>cache.
That can probably be done. But what happened to "protocol writers don't have to
worry about this issues" ideas though ;-) ?
The other thing that I don't like is that to create one socket we'd have to 
call whole bunch of functions to take care of the module refcounting.
So I want to bring these questions again: 
- 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)'.
I'm saying that we don't care whether net_family is registered or not if we know who owns
the socket (i.e. sock->owner).

So my point is yes we can make families to implement their own infrastructure for module
refcounting. But the solution is going to be more complicated than just having sock->owner
and sk->owner fields and allowing for ownership transfers.

>That is, we have a higher layer for net families, with locking for the whole
>family done like it is on the tree now and a lower layer at the specific
>net family, both having the same behaviour at its layers.
>
>This option seems to be easy to implement with the current bluetooth
>infrastructure (i.e. it has a net_families equivalent, it does the switching at
>bt_create time, etc).
Well, we already use sk->destruct call back in Bluetooth protocols (each proto has its 
own call back that does different things). I think we should not use existing sk call 
backs for module refcounting.

Max

Re: New module infrastructure for net_proto_family

[Arnaldo Carvalho de Melo]

Em Tue, Apr 29, 2003 at 12:21:49PM -0700, Max Krasnyansky escreveu:
> At 06:43 PM 4/28/2003, Arnaldo Carvalho de Melo wrote:
> >Em Mon, Apr 28, 2003 at 02:10:11PM -0700, Max Krasnyansky escreveu:

> >> Hmm, no comments on my last email
> >> (http://marc.theaimsgroup.com/?l=linux-netdev&m=105123134301565&w=2) Are
> >> you trying to ignore me too ? ;-)

> >Nope, I was working on the higher layer first, to then come to the net
> >families that are already modular and have per-protocol modules, and I'm
> >starting to look into the only, AFAIK, network family that has this in
> >place: bluetooth :-)

> >Having said that I haven't fully studied this I see two scenarios (brainstorming):

> >1. the net family that wants per protocol "sub" modules "duplicates" the
> >infrastructure having PROTO_sk_alloc and PROTO_destruct (the sk_free
> >sk->destruct hook call), PROTO_sk_alloc uses its net_families equivalent
> >(bt_proto in bluetooth) to find the owner (the "sub" module, i.e. per
> >protocol module) and PROTO_net_family_gets it, then calls sk_alloc proper,
> >and when the last reference to the sock is released the sk->destruct is
> >called (PROTO_destruct) does the PROTO_net_family_put. Ditto for the socket
> >case, where PROTO_create, before calling the ->create of the "sub" module
> >does the PROTO_net_family_get, and at release time its PROTO_release does
> >the same thing that sock_release does. Something like this may well need
> >extra info to be kept at the private area of the proto family in struct sock
> >protinfo or private slab cache.

> That can probably be done. But what happened to "protocol writers don't have
> to worry about this issues" ideas though ;-) ?

They don't, unless they want to have its own infrastructure for loading modules
for each specific protocol, look at all the net families that don't do that
(i.e. all, except pppox and bluetooth, and perhaps ipv4 in the future, but as
you said, this is not accepted by DaveM).

And look at the pppox implementation, it didn't needed any changes to the
struct sock protinfo/private slab cache, and removed code in the protocol
module.

> The other thing that I don't like is that to create one socket we'd have to
> call whole bunch of functions to take care of the module refcounting.  So I
> want to bring these questions again: 

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

Because we don't assume they won't use its own 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)'.  I'm saying that we don't care
> whether net_family is registered or not if we know who owns the socket (i.e.
> sock->owner).

but then we move the overhead to _all_ struct sock, overhead (memory) that is
present at all times, not just at sock creation/destruction time.
 
> So my point is yes we can make families to implement their own infrastructure
> for module refcounting. But the solution is going to be more complicated than
> just having sock->owner and sk->owner fields and allowing for ownership
> transfers.

Have you seen the pppox implementation? It even simplified some things at the
protocol level at the expense of adding just one new exported function at the
net family level and some cost at the struct sock creation/destruction time.

And this by making use of the existing infrastructure to achieve its goals: to
have per protocol modules. And, again, the pppoe protocol module was
simplified.
 
> >That is, we have a higher layer for net families, with locking for the whole
> >family done like it is on the tree now and a lower layer at the specific net
> >family, both having the same behaviour at its layers.

> >This option seems to be easy to implement with the current bluetooth
> >infrastructure (i.e. it has a net_families equivalent, it does the switching
> >at bt_create time, etc).

> Well, we already use sk->destruct call back in Bluetooth protocols (each
> proto has its own call back that does different things). I think we should
> not use existing sk call backs for module refcounting.

Look at what I did in pppox, you will still have similar functionality.

- Arnaldo

Re: New module infrastructure for net_proto_family

[Max Krasnyansky]

At 02:55 PM 4/29/2003, Arnaldo Carvalho de Melo wrote:
>Em Tue, Apr 29, 2003 at 12:21:49PM -0700, Max Krasnyansky escreveu:
>
>> That can probably be done. But what happened to "protocol writers don't have
>> to worry about this issues" ideas though ;-) ?
>
>They don't, unless they want to have its own infrastructure for loading modules
>for each specific protocol, look at all the net families that don't do that
>(i.e. all, except pppox and bluetooth, and perhaps ipv4 in the future, but as
>you said, this is not accepted by DaveM).
Hold on. That was my argument in favor of sk_set_owner() thing. 
I thought the goal was to make it easier in all cases.

>And look at the pppox implementation, it didn't needed any changes to the
>struct sock protinfo/private slab cache, and removed code in the protocol
>module.
But it introduced overhead in sk allocation/deallocation (read me rsp in pppox thread). 

>> The other thing that I don't like is that to create one socket we'd have to
>> call whole bunch of functions to take care of the module refcounting.  So I
>> want to bring these questions again: 
>
>> - Why do we have to bump module refcount for 'struct sock' with _default_ callbacks ?
>
>Because we don't assume they won't use its own callbacks.
And what I'm saying is that if we make this assumption we can save some overhead during
socket allocation/deallocation because we don't have to track those sks.

>> 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)'.  I'm saying that we don't care
>> whether net_family is registered or not if we know who owns the socket (i.e.
>> sock->owner).
>
>but then we move the overhead to _all_ struct sock, overhead (memory) that is
>present at all times, not just at sock creation/destruction time.
Like I said we can easily make space for ->owner in the struct sk (kill one of the 
pprev, prev, bind_prev, etc fields). IMO overhead of calling more function for socket 
allocation/dealocation is greater.

>> So my point is yes we can make families to implement their own infrastructure
>> for module refcounting. But the solution is going to be more complicated than
>> just having sock->owner and sk->owner fields and allowing for ownership
>> transfers.
>
>Have you seen the pppox implementation? It even simplified some things at the
>protocol level at the expense of adding just one new exported function at the
>net family level and some cost at the struct sock creation/destruction time.
>
>And this by making use of the existing infrastructure to achieve its goals: to
>have per protocol modules. And, again, the pppoe protocol module was
>simplified.
I'll comment on that on in pppox thread.

Max