Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Song Liu <songliubraving@fb.com>
To:     Steven Rostedt <rostedt@goodmis.org>
CC:     Song Liu <song@kernel.org>, Networking <netdev@vger.kernel.org>,
        bpf <bpf@vger.kernel.org>, lkml <linux-kernel@vger.kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Kernel Team <Kernel-team@fb.com>,
        "jolsa@kernel.org" <jolsa@kernel.org>,
        "mhiramat@kernel.org" <mhiramat@kernel.org>
Subject: Re: [PATCH v2 bpf-next 3/5] ftrace: introduce
 FTRACE_OPS_FL_SHARE_IPMODIFY
Thread-Topic: [PATCH v2 bpf-next 3/5] ftrace: introduce
 FTRACE_OPS_FL_SHARE_IPMODIFY
Thread-Index: AQHYdrjyXTiiEgEgK0K1BTZfHuJgSq19RUuAgAGMyICAAAmfgIAAFU+AgAALzACAAADvgIAA+XEA
Date:   Fri, 15 Jul 2022 17:42:55 +0000
Message-ID: <0CE9BF90-B8CE-40F6-A431-459936157B78@fb.com>
References: <20220602193706.2607681-1-song@kernel.org>
 <20220602193706.2607681-4-song@kernel.org>
 <20220713203343.4997eb71@rorschach.local.home>
 <AA1D9833-DF67-4AFD-815C-DD89AB57B3A2@fb.com>
 <20220714204817.2889e280@rorschach.local.home>
 <6A7EF1C7-471B-4652-99C1-87C72C223C59@fb.com>
 <20220714224646.62d49e36@rorschach.local.home>
 <170BE89A-101C-4B25-A664-5E47A902DB83@fb.com>
In-Reply-To: <170BE89A-101C-4B25-A664-5E47A902DB83@fb.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?RIR4eAK+WJrhstjUxv1340d1TilaJPR44wr6iaOPEOAtShCpiHr84F5Cw8tJ?=
 =?us-ascii?Q?J2XYMcM2M2ZmUpkYFV57qRGAhGgdOXfPosjFiGQrv4dGUY4rIgP2XvxPEu+S?=
 =?us-ascii?Q?SPB8VN0hJjBKvQCKxTh7SZG4BRxTdvKRkoWL439/Y9mxcEpdzrbBk5ThgOMt?=
 =?us-ascii?Q?TuyRPjqQiOR0YtYL3WuGlCWfVH4eipWSSB1sHwiO9IL7o7KObIYCuYRepEmB?=
 =?us-ascii?Q?XrafcM9gRV6sinDg9Ps0MVZM8FL9XUVaD11IlmkFqOOFyBJ5cK6aa92tTqM/?=
 =?us-ascii?Q?unn28LVX9GoKER6K65rw51t+siOShH/sNdMtw4eHs9J44lJG2so2Gk6IBqgG?=
 =?us-ascii?Q?Tz9UpfpvyoPdh7/Lwrf66mnqR65R3r/jCAxRdO47Gwn7m5AUAnNM/TvdLKI3?=
 =?us-ascii?Q?CcJ4R7g7gYgNQghcAurxnPMUHbLwiN2M+uyzCaNra0F9V2dbKWvO66RXPXp9?=
 =?us-ascii?Q?oH+3YBvlkiTXJAAusq2Tq4ORBQq150taogxT674en71pKplU2vnMe1X1U1qr?=
 =?us-ascii?Q?aFAJP54OXokphKIP3msTEuAca7QyP8td7ckERkThFQnvn7Mbxgh0vzwZj3ZO?=
 =?us-ascii?Q?OaoYdN1ZxHr0Q3NSL6pSa/1RVaQ7xrak8T+yytlVwxjH9mm/8JfZ20o17fSQ?=
 =?us-ascii?Q?9MtA3v8IGr9P6sJ02uEqzYYxyo/YoKK7U6+ygrxEyjOVBVEeOfRHkNq+KsDb?=
 =?us-ascii?Q?dYEfSJ7oIz2V+G4nzUyspCvWLM8UCXOE3sxHUFZy9Bqv3A1X99idMDy8DUdK?=
 =?us-ascii?Q?KahdoOqWCadzTpBZeizv4V3VoL3kdgEvZ5vztMUEQ7aNOa9Xz+0dhLrDCCxU?=
 =?us-ascii?Q?9cbO9JOTMXHThrQ+BOaBJgGmqI6SIcP9danJYj1LMVVAjxqrLLdSyOv624tg?=
 =?us-ascii?Q?9udg9AgV3ORcsMdaFw6K6qn8Akvs3AscTfM/NuC5LJujRA2dhm/HB8v0eBQF?=
 =?us-ascii?Q?fatb1bDiIivxZLFPHtkcCuNTQ/GtLVkxf+I1T4Fquu3rF1gR1HWZykF9XN54?=
 =?us-ascii?Q?C4t0GpK7dS5+4VfB2/pScJt3ByGWaM/27Timsfk88YTruBUONNG3JkQdzjog?=
 =?us-ascii?Q?Hys/1YsNqQTjBNWZBMdHw3VSu2yTrVqJhjNjnsEpIOcBtkgRMxaZ+jH7dTNY?=
 =?us-ascii?Q?zXTLxCnmRjhTQfcyCbxTE+rbBIxV03kSriAGDnvI1pXUxw/B7iBCWOqDH8Fn?=
 =?us-ascii?Q?GMBjm4ROIAR4VttMxzEZyKpfTpH0lKL2F42NPqYoY7U5qFxIX4w+HPdIkE+X?=
 =?us-ascii?Q?DYRd/cmVcvuPiCcggHinOK/OYScXM0iTOaOmTzdGX4nSuPf7G66Trn0oPw/g?=
 =?us-ascii?Q?vgHImnm1HQ5Z8gr3qxSGJqlucCjxWNQkAs2ZMvgyYYlsdGvCaG6LJx1oubKC?=
 =?us-ascii?Q?VNpbThExSkAckmv7SEu4DR+Aw/kegmymKzIKkGXLI67zERFXl+tD56wMENoU?=
 =?us-ascii?Q?fDGKri/v8LqV4tgCyqXmZXQ4TGZKEPh4sE8BV0noBAdpLNdV3jGAphMo39fU?=
 =?us-ascii?Q?QY0xNPmsAXfUsLjUNVxlx0DPu4w8mSUAycyVH7aWThE+j083c1jm9ih/36hl?=
 =?us-ascii?Q?/t20DvsXQ+iVUUYzb9PlcbYb3QrPqxKd51CGKjCTM1awCj9S4e/3FDurZErJ?=
 =?us-ascii?Q?2GEm1Y4stvJFD2II+Z8pv+U=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <D88153F4C499C64BA96409B120F9CF38@namprd15.prod.outlook.com>
MIME-Version: 1.0
X-OriginatorOrg: fb.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SA1PR15MB5109.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: af74fece-263f-4bfb-f009-08da668972ee
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Jul 2022 17:42:55.0848
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Vfs+0uyyVOu0ur2Qm2JkjO9g3kYJaREL5t7VmjM2eD8Wfec2Zd5q3XkgZxX3ot3/atsqj6MPPHhLOnP2BSoekw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR15MB5385
X-Proofpoint-GUID: RkPgpwIJoHw4C61OV3AffVYbIBUmPb4S
X-Proofpoint-ORIG-GUID: RkPgpwIJoHw4C61OV3AffVYbIBUmPb4S
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.883,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-07-15_09,2022-07-15_01,2022-06-22_01
X-Spam-Status: No, score=-2.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Steven,

> On Jul 14, 2022, at 7:50 PM, Song Liu <songliubraving@fb.com> wrote:
> 
> 
> 
>> On Jul 14, 2022, at 7:46 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> 
>> On Fri, 15 Jul 2022 02:04:33 +0000
>> Song Liu <songliubraving@fb.com> wrote:
>> 
>>>> 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). 
>> 
>> My fault. I was probably not being clear when I was suggesting that
>> DIRECT should *act* like an IPMODIFY, but never explicitly stated that
>> it should not set the IPMODIFY flag.
>> 
>> The only reason it does today was to make it easy to act like an
>> IPMODIFY (because it set the flag). But I'm now suggesting to get rid
>> of that and just make DIRECT act like an IPMDOFIY as there can only be
>> one of them on a function, but now we have some cases where DIRECT can
>> work with IPMODIFY via the callbacks.
> 
> Thanks for the clarification. I think we are finally on the same page on
> this. :)

A quick update and ask for feedback/clarification.

Based on my understanding, you recommended calling ops_func() from 
__ftrace_hash_update_ipmodify() and in ops_func() the direct trampoline
may make changes to the trampoline. Did I get this right?


I am going towards this direction, but hit some issue. Specifically, in 
__ftrace_hash_update_ipmodify(), ftrace_lock is already locked, so the 
direct trampoline cannot easily make changes with 
modify_ftrace_direct_multi(), which locks both direct_mutex and 
ftrace_mutex. 

One solution would be have no-lock version of all the functions called
by modify_ftrace_direct_multi(), but that's a lot of functions and the
code will be pretty ugly. The alternative would be the logic in v2: 
__ftrace_hash_update_ipmodify() returns -EAGAIN, and we make changes to 
the direct trampoline in other places: 

1) if DIRECT ops attached first, the trampoline is updated in 
   prepare_direct_functions_for_ipmodify(), see 3/5 of v2;

2) if IPMODIFY ops attached first, the trampoline is updated in
   bpf_trampoline_update(), see "goto again" path in 5/5 of v2. 

Overall, I think this way is still cleaner. What do you think about this?

Thanks,
Song