| Message ID | 20220910154959.15971-1-jgross@suse.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | xen/spinlock: make recursive spinlocks a dedicated type | expand |
On 9/10/22 11:49, Juergen Gross wrote: > Instead of being able to use normal spinlocks as recursive ones, too, > make recursive spinlocks a special lock type. > > This will make the spinlock structure smaller in production builds and > add type-safety. Just a little yak shaving, IMHO a spinlock is normally not expected to be recursive. Thus explicitly naming a spinlock as non-recursive I find to be redundant along with being expensive for typing. Whereas a recursive spinlock is the special instance and should have a declarative distinction. Only codifying the recursive type would significantly cut down on the size of the series and still provide equal type and visual clarification. v/r, dps
On 14.12.22 16:03, Daniel P. Smith wrote: > > On 9/10/22 11:49, Juergen Gross wrote: >> Instead of being able to use normal spinlocks as recursive ones, too, >> make recursive spinlocks a special lock type. >> >> This will make the spinlock structure smaller in production builds and >> add type-safety. > > Just a little yak shaving, IMHO a spinlock is normally not expected to be > recursive. Thus explicitly naming a spinlock as non-recursive I find to be > redundant along with being expensive for typing. Whereas a recursive spinlock is > the special instance and should have a declarative distinction. Only codifying > the recursive type would significantly cut down on the size of the series and > still provide equal type and visual clarification. A "normal" spinlock is non-recursive. A recursive spinlock in Xen can be either taken recursive, or it can be taken non-recursive, causing further recursive attempts to spin. So the explicit non-recursive locking applies to that special treatment of recursive spinlocks. Juergen
On 12/14/22 10:31, Juergen Gross wrote: > On 14.12.22 16:03, Daniel P. Smith wrote: >> >> On 9/10/22 11:49, Juergen Gross wrote: >>> Instead of being able to use normal spinlocks as recursive ones, too, >>> make recursive spinlocks a special lock type. >>> >>> This will make the spinlock structure smaller in production builds and >>> add type-safety. >> >> Just a little yak shaving, IMHO a spinlock is normally not expected to >> be recursive. Thus explicitly naming a spinlock as non-recursive I >> find to be redundant along with being expensive for typing. Whereas a >> recursive spinlock is the special instance and should have a >> declarative distinction. Only codifying the recursive type would >> significantly cut down on the size of the series and still provide >> equal type and visual clarification. > > A "normal" spinlock is non-recursive. > > A recursive spinlock in Xen can be either taken recursive, or it can be > taken > non-recursive, causing further recursive attempts to spin. Yes, I understand the current situation. > So the explicit non-recursive locking applies to that special treatment of > recursive spinlocks. I understand this, but to help clarify what I am saying is that individuals coming from outside Xen would expect is the spinlock family of calls to behave as a non-recursive spinlocks and recursive spinlock family of calls would provide the recursive behavior. Currently Xen's behavior is backwards to this, which this series continues and is a valid approach. Here spinlock and recursive spinlock family of calls are recursive and must use non-recursive spinlock family to have "normal" spinlock behavior. IMHO it would greatly simplify the code and align with the "normal" understanding of spinlocks if instead spin_{lock,locked,unlock} macros were the non-recursive calls and spin_{lock,locked,unlock}_recursive macros were the recursive calls. Then there would only be two suites of calls for spinlocks and a lot less keystrokes for need for most development. v/r, dps
On 14.12.22 17:25, Daniel P. Smith wrote: > On 12/14/22 10:31, Juergen Gross wrote: >> On 14.12.22 16:03, Daniel P. Smith wrote: >>> >>> On 9/10/22 11:49, Juergen Gross wrote: >>>> Instead of being able to use normal spinlocks as recursive ones, too, >>>> make recursive spinlocks a special lock type. >>>> >>>> This will make the spinlock structure smaller in production builds and >>>> add type-safety. >>> >>> Just a little yak shaving, IMHO a spinlock is normally not expected to be >>> recursive. Thus explicitly naming a spinlock as non-recursive I find to be >>> redundant along with being expensive for typing. Whereas a recursive spinlock >>> is the special instance and should have a declarative distinction. Only >>> codifying the recursive type would significantly cut down on the size of the >>> series and still provide equal type and visual clarification. >> >> A "normal" spinlock is non-recursive. >> >> A recursive spinlock in Xen can be either taken recursive, or it can be taken >> non-recursive, causing further recursive attempts to spin. > > Yes, I understand the current situation. > >> So the explicit non-recursive locking applies to that special treatment of >> recursive spinlocks. > > I understand this, but to help clarify what I am saying is that individuals > coming from outside Xen would expect is the spinlock family of calls to behave > as a non-recursive spinlocks and recursive spinlock family of calls would > provide the recursive behavior. Currently Xen's behavior is backwards to this, > which this series continues and is a valid approach. Here spinlock and recursive > spinlock family of calls are recursive and must use non-recursive spinlock > family to have "normal" spinlock behavior. IMHO it would greatly simplify the My series doesn't change treatment of normal spinlocks. They are still used via spin_{lock,locked,unlock}. > code and align with the "normal" understanding of spinlocks if instead > spin_{lock,locked,unlock} macros were the non-recursive calls and > spin_{lock,locked,unlock}_recursive macros were the recursive calls. Then there > would only be two suites of calls for spinlocks and a lot less keystrokes for > need for most development. Only the recursive spinlock type user must specify, whether a lock is meant to be handled as a recursive or a non-recursive lock attempt. This is similar to a rwlock, where the user must specify whether to lock as a reader or a writer. Juergen
On 14.12.2022 17:36, Juergen Gross wrote: > On 14.12.22 17:25, Daniel P. Smith wrote: >> On 12/14/22 10:31, Juergen Gross wrote: >>> On 14.12.22 16:03, Daniel P. Smith wrote: >>>> >>>> On 9/10/22 11:49, Juergen Gross wrote: >>>>> Instead of being able to use normal spinlocks as recursive ones, too, >>>>> make recursive spinlocks a special lock type. >>>>> >>>>> This will make the spinlock structure smaller in production builds and >>>>> add type-safety. >>>> >>>> Just a little yak shaving, IMHO a spinlock is normally not expected to be >>>> recursive. Thus explicitly naming a spinlock as non-recursive I find to be >>>> redundant along with being expensive for typing. Whereas a recursive spinlock >>>> is the special instance and should have a declarative distinction. Only >>>> codifying the recursive type would significantly cut down on the size of the >>>> series and still provide equal type and visual clarification. >>> >>> A "normal" spinlock is non-recursive. >>> >>> A recursive spinlock in Xen can be either taken recursive, or it can be taken >>> non-recursive, causing further recursive attempts to spin. >> >> Yes, I understand the current situation. >> >>> So the explicit non-recursive locking applies to that special treatment of >>> recursive spinlocks. >> >> I understand this, but to help clarify what I am saying is that individuals >> coming from outside Xen would expect is the spinlock family of calls to behave >> as a non-recursive spinlocks and recursive spinlock family of calls would >> provide the recursive behavior. Currently Xen's behavior is backwards to this, >> which this series continues and is a valid approach. Here spinlock and recursive >> spinlock family of calls are recursive and must use non-recursive spinlock >> family to have "normal" spinlock behavior. IMHO it would greatly simplify the > > My series doesn't change treatment of normal spinlocks. They are still used via > spin_{lock,locked,unlock}. > >> code and align with the "normal" understanding of spinlocks if instead >> spin_{lock,locked,unlock} macros were the non-recursive calls and >> spin_{lock,locked,unlock}_recursive macros were the recursive calls. Then there >> would only be two suites of calls for spinlocks and a lot less keystrokes for >> need for most development. > > Only the recursive spinlock type user must specify, whether a lock is meant to > be handled as a recursive or a non-recursive lock attempt. This is similar to > a rwlock, where the user must specify whether to lock as a reader or a writer. While I can't come up with anything neat right away, it feels like it should be possible to come up with some trickery to make spin_lock() usable on both lock types, eliminating the need to ..._nonrecursive() variants visible at use sites (they may still be necessary as helpers then). At least if a spinlock_t instance wasn't embedded in the recursive lock struct (as I did suggest), then something along the lines of what tgmath.h does may be possible. Jan
On 15.12.22 08:48, Jan Beulich wrote: > On 14.12.2022 17:36, Juergen Gross wrote: >> On 14.12.22 17:25, Daniel P. Smith wrote: >>> On 12/14/22 10:31, Juergen Gross wrote: >>>> On 14.12.22 16:03, Daniel P. Smith wrote: >>>>> >>>>> On 9/10/22 11:49, Juergen Gross wrote: >>>>>> Instead of being able to use normal spinlocks as recursive ones, too, >>>>>> make recursive spinlocks a special lock type. >>>>>> >>>>>> This will make the spinlock structure smaller in production builds and >>>>>> add type-safety. >>>>> >>>>> Just a little yak shaving, IMHO a spinlock is normally not expected to be >>>>> recursive. Thus explicitly naming a spinlock as non-recursive I find to be >>>>> redundant along with being expensive for typing. Whereas a recursive spinlock >>>>> is the special instance and should have a declarative distinction. Only >>>>> codifying the recursive type would significantly cut down on the size of the >>>>> series and still provide equal type and visual clarification. >>>> >>>> A "normal" spinlock is non-recursive. >>>> >>>> A recursive spinlock in Xen can be either taken recursive, or it can be taken >>>> non-recursive, causing further recursive attempts to spin. >>> >>> Yes, I understand the current situation. >>> >>>> So the explicit non-recursive locking applies to that special treatment of >>>> recursive spinlocks. >>> >>> I understand this, but to help clarify what I am saying is that individuals >>> coming from outside Xen would expect is the spinlock family of calls to behave >>> as a non-recursive spinlocks and recursive spinlock family of calls would >>> provide the recursive behavior. Currently Xen's behavior is backwards to this, >>> which this series continues and is a valid approach. Here spinlock and recursive >>> spinlock family of calls are recursive and must use non-recursive spinlock >>> family to have "normal" spinlock behavior. IMHO it would greatly simplify the >> >> My series doesn't change treatment of normal spinlocks. They are still used via >> spin_{lock,locked,unlock}. >> >>> code and align with the "normal" understanding of spinlocks if instead >>> spin_{lock,locked,unlock} macros were the non-recursive calls and >>> spin_{lock,locked,unlock}_recursive macros were the recursive calls. Then there >>> would only be two suites of calls for spinlocks and a lot less keystrokes for >>> need for most development. >> >> Only the recursive spinlock type user must specify, whether a lock is meant to >> be handled as a recursive or a non-recursive lock attempt. This is similar to >> a rwlock, where the user must specify whether to lock as a reader or a writer. > > While I can't come up with anything neat right away, it feels like it should be > possible to come up with some trickery to make spin_lock() usable on both lock > types, eliminating the need to ..._nonrecursive() variants visible at use sites > (they may still be necessary as helpers then). At least if a spinlock_t instance > wasn't embedded in the recursive lock struct (as I did suggest), then something > along the lines of what tgmath.h does may be possible. Might be, but do we really want that? Wouldn't it make more sense to let the user explicitly say that he wants a lock to be taken non-recursively? Allowing "spin_lock()" would add some more risk to use it by accident e.g. because of copy/paste without noticing that it is a recursive lock that is taken non-recursively. Same applies for patch reviews. I'd prefer to make this easily visible. Juergen
On 15/12/2022 08:03, Juergen Gross wrote: > Might be, but do we really want that? > > Wouldn't it make more sense to let the user explicitly say that he wants > a lock > to be taken non-recursively? Allowing "spin_lock()" would add some more > risk to > use it by accident e.g. because of copy/paste without noticing that it is a > recursive lock that is taken non-recursively. Same applies for patch > reviews. > > I'd prefer to make this easily visible. FWIW, +1. Cheers,