lkml.org 
[lkml]   [2017]   [Oct]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 1/2] livepatch: Add dynamic klp_object and klp_func iterators
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

\
 
 \ /
  Last update: 2017-10-11 04:52    [W:0.055 / U:7.316 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site