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. 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. > > Does this make sense? Did I miss some better solutions? Did I fill in the holes? ;-) -- Steve
