> On Jul 14, 2022, at 5:48 PM, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote: > > On Fri, 15 Jul 2022 00:13:51 +0000 > Song Liu <songliubraving@xxxxxx> wrote: > >> I think there is one more problem here. If we force all direct trampoline >> set IPMODIFY, and remove the SHARE_IPMODIFY flag. It may cause confusion >> and/or extra work here (__ftrace_hash_update_ipmodify). > > I'm saying that the caller (BPF) does not need to set IPMODIFY. Just > change the ftrace.c to treat IPMODIFY the same as direct. In fact, the > two should be mutually exclusive (from a ftrace point of view). That > is, if DIRECT is set, then IPMODIFY must not be. > > Again, ftrace will take care of the accounting of if a rec has both > IPMODIFY and DIRECT on it. > >> >> Say __ftrace_hash_update_ipmodify() tries to attach an ops with IPMODIFY, >> and found the rec already has IPMODIFY. At this point, we have to iterate >> all ftrace ops (do_for_each_ftrace_op) to confirm whether the IPMODIFY is >> from > > What I'm suggesting is that a DIRECT ops will never set IPMODIFY. Aha, this the point I misunderstood. I thought DIRECT ops would always set IPMODIFY (as it does now). > Like > I mentioned before, ftrace doesn't care if the direct trampoline > modifies IP or not. All ftrace will do is ask the owner of the direct > ops if it is safe to have an IP modify attached to it or not. And at > the same time will tell the direct ops owner that it is adding an > IPMODIFY ops such that it can take the appropriate actions. > > In other words, IPMODIFY on the rec means that it can not attach > anything else with an IPMODIFY on it. > > The direct ops should only set the DIRECT flag. If an IPMODIFY ops is > being added, if it already has an IPMODIFY then it will fail right there. > > If direct is set, then a loop for the direct ops will be done and the > callback is made. If the direct ops is OK with the IPMODIFY then it > will adjust itself to work with the IPMODIFY and the IPMODIFY will > continue to be added (and then set the rec IPMODIFY flag). > >> >> 1) a direct ops that can share IPMODIFY, or >> 2) a direct ops that cannot share IPMODIFY, or >> 3) another non-direct ops with IPMODIFY. >> >> For the 1), this attach works, while for 2) and 3), the attach doesn't work. >> >> OTOH, with current version (v2), we trust the direct ops to set or clear >> IPMODIFY. rec with IPMODIFY makes it clear that it cannot share with another >> ops with IPMODIFY. Then we don't have to iterate over all ftrace ops here. > > The only time an iterate should be done is if rec->flags is DIRECT and !IPMODIFY. Yeah, this makes sense. And this shouldn't be too expensive. > >> >> Does this make sense? Did I miss some better solutions? > > Did I fill in the holes? ;-) You sure did. :) Thanks, Song
