On 01/25, Brandon Williams wrote: > On 01/25, Junio C Hamano wrote: > > Brandon Williams <bmw...@google.com> writes: > > > > >> In my mind, the value of having a constant check_attr is primarily > > >> that it gives us a stable pointer to serve as a hashmap key, > > >> i.e. the identifier for each call site, in a later iteration. > > > > > > We didn't really discuss this notion of having the pointer be a key into > > > a hashmap, what sort of information are you envisioning being stored in > > > this sort of hashmap? > > > > The "entries relevant to this attr_check() call, that is specific to > > the <check_attr instance, the thread> tuple" (aka "what used to be > > called the global attr_stack") we discussed would be the primary > > example. A thread is likely be looping in a caller that has many > > paths inside a directory, calling a function that has a call to > > attr_check() for each path. Having something that can use to > > identify the check_attr instance in a stable way, even when the > > inner function is called and returns many times, would allow us to > > populate the "attr stack" just once for the thread when it enters a > > directory for the first time (remember, another thread may be > > executing the same codepath, checking for paths in a different > > directory) and keep using it. There may be other mechanisms you can > > come up with, so I wouldn't say it is the only valid way, but it is > > a way. That is why I said: > > > > >> But all of the above comes from my intuition, and I'll very much > > >> welcome to be proven wrong with an alternative design, or better > > >> yet, a working code based on an alternative design ;-). > > > > near the end of my message. > > > > > One issue I can see with this is that the > > > functions which have a static attr_check struct would still not be thread > > > safe if the initialization of the structure isn't surrounded by a mutex > > > itself. ie > > > > Yes, that goes without saying. That is why I suggested Stefan to do > > not this: > > > > > static struct attr_check *check; > > > > > > if (!check) > > > init(check); > > > > > > would need to be: > > > > > > lock() > > > if (!check) > > > init(check); > > > unlock(); > > > > but this: > > > > static struct attr_check *check; > > init(&check); > > > > and hide the lock/unlock gymnastics inside the API. I thought that > > already was in what you inherited from him and started your work > > on top of? > > I essentially built off of the series you had while using Stefan's > patches as inspiration, but I don't believe the kind of mechanism you > are describing existed in Stefan's series. His series had a single lock > for the entire system, only allowing a single caller to be in it at any > given time. This definitely isn't ideal, hence why I picked it up. > > Implementation aside I want to try and nail down what the purpose of > this refactor is. There are roughly two notions of being "thread-safe". > > 1. The first is that the subsystem itself is thread safe, that is > multiple threads can be executing inside the subsystem without stepping > on each others work. > > 2. The second is that the object itself is thread safe or that multiple > threads can use the same object. > > I thought that the main purpose of this was to achieve (1) since > currently that is not the case.
Ok, so I discovered a very good reason why we should do as Stefan originally did and split the question and answer (beyond the reasoning for using the reference as a hashkey). One motivation behind making this API thread-safe is that we can use it in pathspec code to match against attributes. This means that a pathspec structure will contain an attr_check member describing the attributes that a pathspec item is interested in. Then the pathspec structure is passed to match_pathspec() as a const pointer. To me, when passing something as 'const' I expect none of the members should change at all. The struct should remain exactly in the same form as before I invoked the function. Requiring the attr_check structure to be modified in the process of a check_attrs() call would violate this "contract" when calling match_pathspec() as the attr_check structure would have modified state. The compiler wouldn't catch this as the "const" modifier isn't passed on to struct members. -- Brandon Williams
