Messages in this thread | | | Date | Wed, 19 Feb 2003 09:45:59 -0800 | From | Max Krasnyansky <> | Subject | Re: [PATCH/RFC] New module refcounting for net_proto_family |
| |
At 07:54 PM 2/18/2003, Rusty Russell wrote: >In message <5.1.0.14.2.20030218101309.048d4288@mail1.qualcomm.com> you write: >> At 07:46 PM 2/17/2003, David S. Miller wrote: >> >> >After talking to Alexey, I don't like this patch. >> > >> >The new module subsystem was supposed to deal with things >> >like this cleanly, and this patch is merely a hack to cover >> >up for it's shortcomings. > >I don't quite understand. > >There are some issue with this patch, however. > >Firstly, the owner field should probably be in struct proto_ops not >struct socket, where the function pointers are. struct proto_ops doesn't exists on its own without struct socket. I think it make sense to simply keep track of the sockets but I don't see any problem with putting it in proto_ops.
struct sock is different though. callbacks are inside.
>The sk thing looks reasonable at first glance. Getting a reference to >npf->owner, then holding it for the socket is a little confusing, but >an obvious optimization over a naive "get, use, drop, get". That was an optimization indeed. There is no point in dropping reference there.
>In sys_accept: > >> @@ -1196,9 +1198,13 @@ >> if (!(newsock = sock_alloc())) >> goto out_put; >> >> - newsock->type = sock->type; >> - newsock->ops = sock->ops; >> + newsock->type = sock->type; >> + newsock->ops = sock->ops; >> + newsock->owner = sock->owner; >> >> + try_module_get(sock->owner); >> + newsock->owner = sock->owner; >> + >> err = sock->ops->accept(sock, newsock, sock->file->f_flags); >> if (err < 0) >> goto out_release; > >You still need to check the result of try_module_get, and fail if it >fails. The *only* time this will fail is when someone is doing an >"rmmod --wait" on the module, which presumably means they really do >not want you to increase the reference count furthur. Ohh, I see. My assumption here was that we know for sure that module is alive at this point since we already hold a reference to the first socket. Actually I was going to send another email and ask for unconditional module_get() specifically for the cases like that.
Even after your explanation I still think we need unconditional module_get() there. Because in this case 'rmmod --wait' will simply brake accept() logic. I mean it'll keep waiting until listening socket is destroyed (i.e. until socket app is killed) but accept() will mysteriously fail for no good reason. Comments ?
Max
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |