Hi Nils, Nils Goroll wrote: > Dai, > >> I think we can achieve the same result with a simpler change >> by removing and re-inserting the entry to the head of the list >> once it was selected. The webrev for that change is here: >> >> http://cr.opensolaris.org/~dain/6817942/ >> <http://cr.opensolaris.org/%7Edain/6817942/> > > First of all, sorry for the delay which was on my side this time. > > Regarding your suggested change: > > * Besides clnt_max_conns, there are other static declarations, which > need to be removed. Your proposed changed does not fix these. Yes, this webrev was intended to show the proposed change for the load balancing issue. I removed only the static declaration for clnt_max_conns for testing purpose. > > * The change you propose changes the way the last used cm_xprt entry > is saved - but this does not have anything to do with the root cause > of the issue, which is that using timestamps is inappropriate. I do > not like the fact that your change exploits an unintentional side > effect of the original solution (the fact that the last of the entries > with equivalent x_time is selected) to implement round robin behavior > without doing away the the flawed implementation in the first place. You can look at this as an exploitation, or compensation for a defect, or fixing a flaw, etc. To me it's the end effect with minimal risk. > > On a meta-layer, I think we can come up with hundreds of alternative > solutions to this issue. I have asked for reviews of my proposed > solution and I have not received any. What was your motivation to > propose a completely different approach? My main concern is the stability and performance of the system when changes are made to a sensitive area such as rpcmod. Simpler change is easier to understand and verify, less chance of introducing new bug, less impact to performance of the code. > > I would also like to point our that I have reduced the complexity for > dooming entries (will be done in one go trough the list rather than > starting all over when an entry is doomed), that I hope to have made > the function more concise and that I hope to improve on maintenance by > adding comments. I'm not sure if we need to address this since clnt_max_conns is not expected to change frequently.
Thanks, -Dai
