ntirpc
ntirpc copied to clipboard
nfs-ganesha
Send queues
This patch is based on Ganesha2.5 libntirpc. Based on the comments, I will update the patch for 'next' branch.
On 9/18/18 2:57 PM, Daniel Gryniewicz wrote:
Okay. The last patch (the change to the output threads) looks fine.
On the other hand, I disagree.
We don't usually make changes to the binary API as a minor update. Please leave xp_ifindex in place.
The one line patch that I've posted to nfs_ganesha could also be used here as a default. xprt->xp_ifindex = xprt->xp_fd;
Originally I'd done the ifindex code here, but somebody wanted it moved to nfs_ganesha. Then it never went in. That's why it's always 0.
send_queue_hash() isn't really a hash. It's a mask. (So just change that line to &ioq_ifqh[svc_ioq_mask(xprt->xp_ifindex)];
Please use svc_ioq_ prefix on the function names.
This isn't scalable. You're making one queue per thread. That's probably better than one queue per fd, but you're talking about a default 512 queues.
I suppose it will be fine on your 4K thread 200GB machines.
Remember, this doesn't actually improve anything. It just randomizes which queues stall, and which order the packets are sent. As the comment section says:
Ideally, these would be some variant of weighted fair queuing.
Also, don't delete the comment section. That's just petty. It's there for information to future developers. In fact, the 2nd and 4th are still correct with your code. The 3rd should simply remove "static".
Enough comments for now. Looking forward to an update.
This isn't scalable. What is the environment where this is NOT scalable? and why? Also, the number of threads is configurable, so this should be as scalable as the existing code (except, I don't think we can have just one queue with this patch, but I can fix that if that is important).
but you're talking about a default 512 queues. Actually 256 on ganesha2.5 due to 200 max default threads. Did we change it in ganesha2.7?
Also, don't delete the comment section. I put it back but it wasn't true before and it is not true now either as there is no true interface index in either code.
Default is still 200 on 2.7, so 256.
I don't have a strong opinion on leaving or removing if_index; it's completely unused, though, so I don't mind it being removed now, because it will otherwise hang around forever.
On 9/18/18 2:57 PM, Daniel Gryniewicz wrote: Okay. The last patch (the change to the output threads) looks fine. We don't usually make changes to the binary API as a minor update. Please leave xp_ifindex in place.
I don't see a need to keep an unused interface either. The ABI changte restriction you mention doesn't reflect any consensus I know of (and it would have to to be real).
Bill raised an interesting point: You're setting it to the next power of 2 up, but that will be more than the max number of threads. It should probably be the next power of 2 down, if it needs to be a power of 2. Or, use a proper hash function that doesn't care.
A better solution, I think, is to have a separate config option for the ioq size, rather than reusing the thread max (since those threads need to be used for recv and processing as well)
