This has a side effect of eliminating filedesc slock/sunlock during path lookup, which in turn removes contention vs concurrent modifications to the fd table.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
also please put a comment in the pwd structure describing how the synchronization works. I believe I understand but am not 100%.
| sys/kern/kern_linker.c | ||
|---|---|---|
| 2068 ↗ | (On Diff #69042) | linker_root_mounted() seems like a more accurate name. |
| sys/sys/filedesc.h | ||
| 157 ↗ | (On Diff #69042) | Why is it useful to have distinct LOCKED and XLOCKED accessors? Just so we can strengthen the assertion? It seems like overkill to me since the distinction doesn't matter wrt to SMR. |
| sys/sys/filedesc.h | ||
|---|---|---|
| 157 ↗ | (On Diff #69042) | I like that the intent is expressed here. There is no problem stemming from having this macro. |
| sys/sys/_smr.h | ||
|---|---|---|
| 50–54 ↗ | (On Diff #69239) | Don't we only really need this in other headers? Do we need the accessors too? |
| 150 ↗ | (On Diff #69239) | Can you do else /* !KERNEL */ above this. |
| sys/sys/filedesc.h | ||
| 95 ↗ | (On Diff #69239) | thank you for converting |
| 155 ↗ | (On Diff #69239) | I wonder if there is a fancier way to express this. |
Comment Actions
Overall this is nice and a better first example of smr than radix.
| sys/kern/kern_descrip.c | ||
|---|---|---|
| 2350 ↗ | (On Diff #69239) | Is there no assert here? And if not, does it need to be serialized? Is a barrier necessary? For this kind of synchronization I would like to try to reinforce either documenting with assert or comment what is expected. |
| 3304 ↗ | (On Diff #69239) | Are we guaranteed by the write order that we won't spin? The entry is cleared before ref reaches zero? |
| 3317 ↗ | (On Diff #69239) | This is for const prop? |
| sys/sys/filedesc.h | ||
| 81 ↗ | (On Diff #69239) | can you add all fields are const after initialization? |
| sys/sys/smr.h | ||
| 98–99 ↗ | (On Diff #69239) | If we move these out I would want a stronger comment directing readers. Or possibly even make this sys/smr.h and the rest sys/smrapi.h or smr_int.h or something. Let's discuss. |
| sys/sys/filedesc.h | ||
|---|---|---|
| 91 ↗ | (On Diff #69239) | Is there any reason we can't have an anonymous smr pointer type, akin to what TAILQ_ENTRY and TAILQ_HEAD do? For radix we need a type, but I don't see its purpose here. |
| 157 ↗ | (On Diff #69042) | My point is that the intent doesn't matter from the perspective of SMR, but the existence of separate macros makes it seem otherwise. |
| sys/sys/smr.h | ||
| 98–99 ↗ | (On Diff #69239) | SMR queue macros will have to go somewhere as well. _smr.h seems like the wrong place if it's going to continue growing, so I like the idea of having a separate header. smr_accessors.h maybe. |
| sys/sys/filedesc.h | ||
|---|---|---|
| 91 ↗ | (On Diff #69239) | We could also add a SMR_POINTER(struct foo) bar or something. |
| 157 ↗ | (On Diff #69042) | I'm not totally sure I understand the objection either. I guess in some sense we are using the smr assert for a dual purpose here but I think this is in the spirit of the thing. |
| sys/sys/smr.h | ||
| 98–99 ↗ | (On Diff #69239) | Smr as it stands is more user facing already. There is very little in the way of implementation exposed other than the structs and I would prefer to keep smr_enter() inline. For a case like pwd that really wants these macros we might just bend the header rules and let it include sys/smr.h. Many system headers include sys/queue.h already. |
| sys/kern/kern_descrip.c | ||
|---|---|---|
| 2350 ↗ | (On Diff #69239) | Here the struct is never used, should be clear from the context. |
| 3304 ↗ | (On Diff #69239) | The code replaces the struct, releases the lock and only then drops the old one. Meaning the replacement is published before the old object is freed. This is not going to spin practice, even if several threads continuously keep changing pwd. For that to loop more than once pwd_hold consumer would have to keep getting incredibly unlucky with interrupts getting in the way so that it never manages to catch a non-zero object. |
| 3317 ↗ | (On Diff #69239) | If you mean to avoid a trip through memset in uma and let the compiler generate (and elide) some movs here, yes. In fact I would argue the M_ZERO flag for uma should be avoided. |
| sys/sys/_smr.h | ||
| 50–54 ↗ | (On Diff #69239) | We do, see pwd_set which is an inline in filedesc.h otherwise i would have to code it up as a macro or a function. |
| sys/sys/filedesc.h | ||
| 91 ↗ | (On Diff #69239) | An anonymous type will be probably useful, but for a consumer like this it would just remove type checking by the compiler. As it is the difference to the stock kernel is that apart from the type being checked we also assert on smr. |
| 155 ↗ | (On Diff #69239) | there probably is but i think this falls beyond the scope of this patch. should someone come up with a good idea this is easy to convert. |
| 157 ↗ | (On Diff #69042) | these macros are intended to be used with struct filedesc and all patched xlocked consumers intend to replace the original. In particular should the code get refactored to get a pointer to fdp, it can avoid: FILEDESC_XLOCK(fdp); pwd = FILEDESC_LOCKED_LOAD_PWD(fdp); I don't think it's a significant deal one way or the other and I prefer this style. |
| sys/sys/smr.h | ||
| 98–99 ↗ | (On Diff #69239) | I have no opinion, just a requirement that I can use these in filedesc.h without trouble which on stock kernel is not possible due to inlines in the smr.h header. Attempts to add missing headers run into drastic issues, thus the initial _smr.h approach. Tl;dr I would prefer if someone dealt with the problem however they see fit, as long as the above requirement is met. It's definitely going to cause trouble for others. |
| sys/sys/filedesc.h | ||
|---|---|---|
| 157 ↗ | (On Diff #69042) | I don't object, but I don't really see the point of having multiple macros to do the same thing. If the idea is to be as specific as possible, why don't we also have FILEDESC_SLOCKED_LOAD_PWD()? It just seems a bit confusing to me. |
| sys/sys/smr.h | ||
| 98–99 ↗ | (On Diff #69239) | It might become trickier over time. smr_enter_preempt() uses sched_pin(), which pulls in sched.h, for instance. Maybe the solution there is to just not have it be an inline. The SMR accessors and type declaration macros are much more likely to be used in a header than smr_enter(), so I do think it is preferable in the long term to have them be in separate headers, whether that's _smr.h or something else. |
| sys/sys/filedesc.h | ||
|---|---|---|
| 91 ↗ | (On Diff #69239) | I can't see the advantage of having smrpwd_t instead of struct filedesc {
...
SMR_POINTER(struct pwd *) fd_pwd;
...with D23988. What type-checking does it provide? |
| sys/sys/smr.h | ||
| 98–99 ↗ | (On Diff #69239) | Ok, I added smr_types.h in D23988. I also changed SMR_TYPE_DECLARE a bit. |
| sys/sys/filedesc.h | ||
|---|---|---|
| 155 ↗ | (On Diff #69239) | I have a patch which adds a #define __smr_ex_eval(x) __builtin_choose_expr(__builtin_types_compatible_p(__typeof(x), void), ((x), 1), (x)) so the expression can be a void function call and you don't need the ", true". I tried to do it with _Generic but I don't think it's possible, you can't match on "void". |
| sys/sys/filedesc.h | ||
|---|---|---|
| 155 ↗ | (On Diff #69239) | ok, but then the comment which gives an example with mtx_owned somewhere in smr headers should be augmented. that is, the recommendation should be to use the actual assertion routine and only resort to directly testing something if that's absent. given that asserts here got abstracted into few places i think this can be patched later after the above is sorted out. I'm trying to reduce my patch queue due to the fast path lookup work. |