On Tue, Oct 10, 2017 at 11:15:33AM -0400, Jason Baron wrote: > > > On 10/06/2017 05:22 PM, Josh Poimboeuf wrote: > > On Wed, Sep 27, 2017 at 11:41:29PM -0400, Jason Baron wrote: > >> In preparation to introducing atomic replace, introduce iterators for > >> klp_func and klp_object, such that objects and functions can be dynamically > >> allocated (needed for atomic replace). This patch is intended to > >> effectively be a no-op until atomic replace is introduced. > > > > Hi Jason, > > > > Sorry for coming in so late on this. This will be a nice feature. I > > haven't gotten to the second patch yet, but I have a few comments > > already. > > > > > Hi Josh, > > > 1) Having both static *and* dynamic lists seems to add a lot of > > complexity and brittleness. > > > > The livepatch API is easier to use with static data structures, so I > > don't think we should change the API. But I also don't think we > > should allow the API to overly constrain our internal representation > > of the data. > > > > What I'd *really* like to do is completely separate the user-supplied > > data structures from our internal data . At registration time, we > > can convert all the arrays to dynamic lists, and then never touch the > > arrays again. Then managing the lists is completely straightforward. > > > > That would also allow us to have an external public struct and an > > internal private struct, so the interface is more clear and we don't > > have to worry about any patch modules accidentally messing with our > > private data. > > > > Separating the external/internal data structures makes sense. Another > idea here would be to simply walk the static data structures at register > time and then add them onto the dynamic lists. Then, all subsequent > access can be done using the simple list walks. That would remove the > complex iterators here. The external/internal division could still be > done later but maybe doesn't couple as much to this patchset... Sounds good. > > 2) There seems to be a general consensus that the cumulative "replace" > > model is the best one. In light of that, I wonder if we can simplify > > our code a bit. Specifically, can we get rid of the func stack? > > > > If the user always uses replace, then the func stack will never have > > more than one func. And we can reject a patch if the user doesn't > > use replace when they're trying to patch a function which has already > > been patched. > > > > Then the func stack doesn't have to be a stack, it can just be a > > single func. > > > > That would allow us to simplify the code in several places for the > > common case, yet still allow those who want to apply non-cumulative > > patches to do so most of the time. > > > > It's not a *huge* simplification, but we've already made livepatch so > > flexible that it's too complex to fit everything in your head at the > > same time, and any simplification we can get away with is well worth > > it IMO. And once we have replace, I wonder if anybody would really > > miss the func stack. > > > > I think that means instead of having a list of functions or the func > stack, you have 2 pointers - one to the active patch and one to the > previous patch. Maybe it would make sense to do in steps? IE have this > patchset reject patches that patch a patch to existing function, leaving > the func stack. And if that doesn't break any use-cases, switch to a > model where there is just an active and previous patch? Hm. Maybe I didn't think it through. I guess the func stack could have at most *two* funcs -- not one. Let's just forget this idea for now... -- Josh -- To unsubscribe from this list: send the line "unsubscribe live-patching" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
