Message ID | 4ED8C940.20509@linux.vnet.ibm.com |
---|---|
State | Rejected, archived |
Delegated to: | David Miller |
Headers | show |
Hello, Srivatsa. On Fri, Dec 02, 2011 at 06:19:04PM +0530, Srivatsa S. Bhat wrote: > So how about solving this problem more fundamentally, such as defining a > freezable wrapper over kernel_recvmsg like: > > #define kernel_recvmsg_freezable(sock, msg, vec, num, size, flags) \ > ({ \ > kernel_recvmsg(sock, msg, vec, num, size, flags) \ > try_to_freeze(); \ > }) > > and using it instead of kernel_recvmsg(), throughout the kernel? > > But kernel_recvmsg is an exported symbol. So if we are very very unwilling > to change the kernel ABI, we could probably think about adding try_to_freeze() > inside kernel_recvmsg itself,like this (but see below about my thoughts about > which one is better): I don't necessarily object to introducing the wrapper but I don't really think we should be doing s//g over the source tree without understanding where it's actually necessary. For kernel threads and user threads out of the signal delivery path, try_to_freeze() is an exceptional event which introduces behavior which can be difficult to reproduce track down and spreading it without actually knowing what the surrounding code is doing doesn't sound like a good idea to me. Thank you.
On 12/05/2011 11:11 PM, Tejun Heo wrote: > Hello, Srivatsa. > > On Fri, Dec 02, 2011 at 06:19:04PM +0530, Srivatsa S. Bhat wrote: >> So how about solving this problem more fundamentally, such as defining a >> freezable wrapper over kernel_recvmsg like: >> >> #define kernel_recvmsg_freezable(sock, msg, vec, num, size, flags) \ >> ({ \ >> kernel_recvmsg(sock, msg, vec, num, size, flags) \ >> try_to_freeze(); \ >> }) >> >> and using it instead of kernel_recvmsg(), throughout the kernel? >> >> But kernel_recvmsg is an exported symbol. So if we are very very unwilling >> to change the kernel ABI, we could probably think about adding try_to_freeze() >> inside kernel_recvmsg itself,like this (but see below about my thoughts about >> which one is better): > > I don't necessarily object to introducing the wrapper but I don't > really think we should be doing s//g over the source tree without > understanding where it's actually necessary. For kernel threads and > user threads out of the signal delivery path, try_to_freeze() is an > exceptional event which introduces behavior which can be difficult to > reproduce track down and spreading it without actually knowing what > the surrounding code is doing doesn't sound like a good idea to me. > Yeah, I agree. But I remember seeing almost _exactly_ the same code as CIFS loop in some other place. I'll see if I can track it down and also understand if it might cause problems. If I find it to be worth it to use the same solution as above, I'll try adding the wrapper and using it there. Else, better to keep it as it is, as you mentioned. Thank you. Regards, Srivatsa S. Bhat -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 5, 2011 at 12:09 PM, Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> wrote: > On 12/05/2011 11:11 PM, Tejun Heo wrote: > >> Hello, Srivatsa. >> >> On Fri, Dec 02, 2011 at 06:19:04PM +0530, Srivatsa S. Bhat wrote: >>> So how about solving this problem more fundamentally, such as defining a >>> freezable wrapper over kernel_recvmsg like: >>> >>> #define kernel_recvmsg_freezable(sock, msg, vec, num, size, flags) \ >>> ({ \ >>> kernel_recvmsg(sock, msg, vec, num, size, flags) \ >>> try_to_freeze(); \ >>> }) >>> >>> and using it instead of kernel_recvmsg(), throughout the kernel? >>> >>> But kernel_recvmsg is an exported symbol. So if we are very very unwilling >>> to change the kernel ABI, we could probably think about adding try_to_freeze() >>> inside kernel_recvmsg itself,like this (but see below about my thoughts about >>> which one is better): >> >> I don't necessarily object to introducing the wrapper but I don't >> really think we should be doing s//g over the source tree without >> understanding where it's actually necessary. For kernel threads and >> user threads out of the signal delivery path, try_to_freeze() is an >> exceptional event which introduces behavior which can be difficult to >> reproduce track down and spreading it without actually knowing what >> the surrounding code is doing doesn't sound like a good idea to me. >> > > > Yeah, I agree. But I remember seeing almost _exactly_ the same code > as CIFS loop in some other place. I'll see if I can track it down and > also understand if it might cause problems. If I find it to be worth it > to use the same solution as above, I'll try adding the wrapper and using > it there. Else, better to keep it as it is, as you mentioned. Thank you. Agreed ...
diff --git a/net/socket.c b/net/socket.c index 2877647..8d80e4f 100644 --- a/net/socket.c +++ b/net/socket.c @@ -88,6 +88,7 @@ #include <linux/nsproxy.h> #include <linux/magic.h> #include <linux/slab.h> +#include <linux/freezer.h> #include <asm/uaccess.h> #include <asm/unistd.h> @@ -774,6 +775,13 @@ int kernel_recvmsg(struct socket *sock, struct msghdr *msg, msg->msg_iov = (struct iovec *)vec, msg->msg_iovlen = num; result = sock_recvmsg(sock, msg, size, flags); set_fs(oldfs); + + /* + * We might sleep in the above code for a long time (a couple of + * seconds), which could cause freezing failures if the caller calls us + * in a loop. So cooperate with the freezer here itself. + */ + try_to_freeze(); return result; } EXPORT_SYMBOL(kernel_recvmsg);