On Thu, 10 Aug 2017, Jason Baron wrote: > > > On 08/10/2017 07:12 AM, Miroslav Benes wrote: > > > > > Ok - associating the "atomic replace" with the patch itself makes sense to > > > me. > > > It would also basically work, I think with the patch I proposed except for > > > the > > > case where the the "atomic replace" was on top of several non-"atomic > > > replace" > > > patches. The reason is that the "atomic replace" I posted looks back 1 > > > patch > > > to see what it needs to replace (assuming all patches are in atomic > > > replace > > > mode). So instead of just looking back 1 patch, it could 'look back' and > > > make > > > sure it was replacing all previously loaded patches. > > > > Yes, this should not be a problem to implement with the current code. > > > > But I must say I am not entirely happy with the code as it is. The big > > advantage is that there are almost no changes to the consistency model. > > Almost everything is created and allocated during the initialization and > > the patch then looks ordinary. The current code can handle it smoothly. > > "Dynamic" superstructure is what I am not happy with. It is too > > complicated in my opinion. On the other hand I don't see a simple way out > > now. > > Indeed. It is possible create the 'no-op' functions statically and relative to > a previous livepatch module at build time, but I didn't like this approach > because it means that the created 'no-op' functions are tied to a specific > prior livepatch module, which it is potentially also inconvenient to keep > around. So for me, the dynamic creation of the 'no-op' functions is important. Yes, I agree. I think the feature should be transparent for the user (livepatch author). Otherwise, its benefits are questionable. > Given that premise, the posted patch adds an additional linked list head to > klp_patch (to add the dynamic klp_objects), and an additional linked list head > to the klp_object (to add the dynamic klp_funcs). It hides this mostly behind > the klp_for_each_object() and klp_for_each_func(), such that a lot of the > code does not need to be adjusted. Note as well that I think in general the > lists of 'no-ops' should be fairly small, and that as soon as the live patch > module is enabled the linked lists are removed and thus the livepatch module > looks the same as before (only contains the static allocations). > > > > > 1. We can try to make your code "nicer". > > > > Sure - I'm happy to incorporate any suggestions, and I could re-review to try > and simplify further. > > > > 2. The main problem is that all user structures (klp_func, klp_object, > > klp_patch) are statically allocated in a kernel module. You need to add > > dynamically allocated structures, which is not easy. We can either change > > everything to dynamic allocation, or make everything static. As far as the > > former is concerned, we've been down that road IIRC. It didn't end up > > well. Is the latter even possible? > > > > I think making everything static is possible - but I think it ties things to > the previously loaded module, which I don't think is desirable. Agreed. > That said we > could also potentially reallocate() the memory for the static regions and > increase them to the required size (although it may be a little tricky to free > the original static region b/c other data structures may be stored nearby?). Yes, that is what I meant above and I am not sure we can/should do it. > I > think its also nice that the current patch frees these 'no-op' functions when > no longer needed, so by keeping them separate, this is somewhat simpler. True. > > 3. We could bypass everything and register special no-op fops to each > > reverted function. Those could be held and handled separately. It could be > > simpler, but it might be problematic to deal with the consistency model > > interaction. > > > > Perhaps...I sort of feel like this moves us closer to handling these > differently, whereas the proposal hides a lot of this behind > klp_for_each_object() and klp_for_each_func(). Ok, I'll take the second look and maybe my impression would be different. I'll try to come up with something. Miroslav -- 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
