In general we shouldn't really rename sysctls. That would break scripts, sysctl.conf configurations, etc..

I think this timeout is kind of bogus anyway, I suspect it's a workaround for some hangs which I fixed in commit 1f6f247b3526abd4d5927684735012220aefe373.

We should validate this more carefully, the user should get an error if they specify port 424242 or whatever.

What's the purpose of having a dedicated thread? 9p is a synchronous protocol. Each thread which submits a request can block directly on the socket rather than going through the indirection of an extra thread.

Rather than having a timeout, I suspect it'd be better to specify PCATCH here, so that a blocked thread can be interrupted by a signal. You'll get that for free if you let request threads block in soreceive().

I don't really agree with this naming: there's nothing here that's inherent to INET sockets (except for a couple of TCP options which can be made conditional on the protocol family). This transport could work just as well with unix sockets. Can we call this a "socket" transport instead?

Is there any real reason to have a separate directory for this? Do you plan to add more transports? If not I'd suggest just calling this file p9_transport_socket.c and keeping it in the main directory.

I don't think there's any real reason to have a separate kernel module here. I understand that the virtio transport has one, but here you are just using the core kernel's interfaces, and p9fs is already a separate module. It is okay to just have this transport be built in to the p9fs module.

There is no need to apologize.

Is it possible to have error == 0 here? I think not.

Why do you use the lowat threshold here? Why not read as soon as there are sizeof(hdr) bytes available?

Ok, I see that you set SO_RCVLOWAT, but I'm not sure why. You are only reading from the socket once that many bytes are already available, so it doesn't matter. RCVLOWAT is basically "the number of bytes that need to be present in the receive socket buffer before soreceive() will return them." I believe the default is 1.

So, in FreeBSD, explicit CVs are basically never needed. You can sleep directly on the lock using mtx_sleep(). You can keep it here if you prefer but I don't think it's really needed.

Better to explicitly state what those reasons are.

Note, strtol() returns a long, not an int.

0 is also an invalid port number, but I guess soconnect() will catch that.

Even with a PCATCH, if a bad server decided to drop response for a request, it would never exit sleep unless interrupted (hence the timeout).

If you want a timeout, note that you can set the SO_RCVTIMEO socket option for this purpose.

Having multiple threads sbwait() on the same lock will cause all of them to wake up at once

Yes, that's fair. Any time data arrives on the socket, all the threads will be awoken.

I think you could work around this by having threads block on the individual request structure, rather than using sbwait(). The socket upcall could figure out which pending request is receiving a response, and then wake up only that thread.

Having a separate thread was cleaner for both the design and implementation to me (especially for error handling, like disconnecting the socket).

Ok. I'm fine with keeping it that way for now.

I don't know though. I would have to run a performance measure against the socket receive buffer locking one.

My gut says that it won't matter very much in practice. Any time you have N threads trying to interact with a shared queue, the queue will be a bottleneck once N is big enough. 9pfs is fundamentally not going to scale well to many threads since you only get one channel to communicate with the server; the virtio transport has the same problem. Moving some work into a dedicated thread might help push the bottleneck down, but it's still there.

Note that indirection through a worker thread also imposes a latency penalty: whenever data is received on the socket, the upcall wakes up the worker thread, which reads data from the socket buffer and then wakes up the original requestor. That indirection is not free; going to sleep and waking up involves making trips through the scheduler. I'm not saying it's the wrong tradeoff here, it's just something to be aware of.

To be honest this sounds like busy-work to me. Is there some specific problem that would addressed by splitting up the modules this way? Are you planning to add some additional transports? If not, I'd just keep things simple.

I think adding it directly from p9fs_init() is fine.

Extra line here.

This file is still using the in9p_ prefix everywhere, it should be socket9p or sock9p or so.

This list doesn't appear to actually get used. Do you have some plan for it?

PCATCH means, "wake me up if a signal is delivered to this thread while I'm asleep," but this is a kernel thread that doesn't receive signals.

You shouldn't manipulate socket state directly like this. Why do you need to clear this flag? The loop should look like kern_connectat().

(At some point we should factor this code out into a helper function. There are several other soconnect() callers which have nearly identical loops.)

Why clear the error? If there's a connection error shouldn't we clean everything up and free the socket?

I guess it's subjective, but this kind of comment is useless IMO.

Right, it was only there for module load/unload purposes.

I read it in the socket(9) man page.

If soconnect() fails, the caller must manually clear the SS_ISCONNECTING flag.

error is checked below, and will eventually lead to a soclose and cleanup.

Can we get rid of it then?

Oof, ok. I am not sure if that's really correct, but now that you say it, I do see that pattern elsewhere, e.g., in clnt_vc_create(). It's probably ok to leave it for now then.

Are you sure? My reading of p9_client_create() is that if the transport's create() method fails, we'll just free the client structure and return without doing any cleanup. Where does the soclose() come from?

sock9p_create() calls sock9p_sock_create(). If it fails, it goes to (err). There, if the socket is not null it is closed and the channel is freed.

It'd be nice to deal with this TODO.

Extra newline here.

Ouch, so anyone can break the channel by sending ctrl-C while a process is blocked here? I don't think that's really workable.

If I'm reading correctly, we can defer reuse of the tag by setting req->tc.tag = P9_NOTAG. See p9_free_req(). Can we handle signals by allowing the requesting thread to return, but clearing the tag and letting the transport layer free it later once we get a reply?

I don't think this can be const, in fact, since p9_register_trans() will modify it when inserting into the transport list.

Ok, I think I assumed that sock9p_sock_create() was the transport's create() implementation, sorry for the noise.

(But I still don't see why we should set so->so_error = 0.)