Hi Abhijeet, On Mon, Apr 15, 2024 at 09:48:25PM -0700, Abhijeet Rastogi wrote: > Hi Willy, > > Thank you for your patience with my questions.
You're welcome! > > It happens that the global struct is only changed during startup > > I used cli_parse_set_maxconn_global as a reference for my patch and my > understanding is, it does have a race as I do not see > thread_isolate(). > > https://github.com/haproxy/haproxy/blob/fa5c4cc6ced5d8cac2132593ed6b10cf3a8a4660/src/cli.c#L1762 Ah I see. I didn't remember we could change it at run time and it predates threads. In practice there's no race since it writes only a single value. It should theoretically use an atomic write here to do things more cleanly but in reality all the platforms we support are 32-bit and none will make non-atomic writes to an aligned 32-bit location. In your case the difference is that you need to set both a value and a flag and need to make them appear consistently to observers, which is why you'd need this isolation. > >That's easier than it seems if you set an ormask, andnotmask and the new > >value from a local variable. > > Correct, however, I wasn't sure if it's okay to read the mask first, > then get out of the critical section to do other stuff. (in case it's > modified by other threads) What I meant was: - parse the cmd line to local variables (value, ormask, andnotmask) - thread_isolate() - apply these local variables to global ones - thread_release() > Also, it felt natural to minimize the use of thread_isolate(), hence, > I went with a solution where we only use that once per execution. I > will fix this if it doesn't make sense. I'd also want to see only one :-) But the way you've done it is fine, a single one is executed for each "if" branch and the code remains clean enough. > >it would be better to call this "set global close-spread-time" > > Good point, I've done it in the latest iteration. Thanks. However you should not reindent the whole table like this in the same patch, it makes the review way more complicated, and even later if for any reason we need to recheck that patch, this part is horrendous. The right approach when you feel like you need to reindent a table due to some longer keywords, function names or so, is to make a preliminary "cleanup" patch that only does the reindent and promises in the commit message that nothing else was touched, and do you feature patch after that one. This way the first one doesn't deserve any analysis and the second one is clean. I'm seeing that an entry is missing for "doc/management.txt" regarding this new command, please write a few lines about it. Otherwise it's generally OK for me. Thank you! Willy
