On 2024/1/18 2:54, Willem de Bruijn wrote:

I agree. A concern with CONFIGs is that what matters in practice is
which default the distros compile with. In this case, adding hurdles
to using the feature likely for no real reason.

Static branches are used throughout the kernel in performance
sensitive paths, exactly because they allow optional paths effectively
for free. I'm quite surprised that this issue is being raised so
strongly here, as they are hardly new or controversial.

The new or controversial part about its usage in the devmem patchset as my
understanding is:
1. It is assumed the devmem and normal page processing in networking does
   not have to be treated equally in the same system, either the performance
   of devmem is favored or the performance of normal page is favored. I think
   if distros is starting to worry about the CONFIG for devmem, devmem must be
   quite popular that we might need the best performance of both. IMHO, static
   branches might be just a convenient way to start supporting the devmem for
   now as we seems to not have a clear idea of unified handling or proper API
   for both devmem and normal page.

2. Specifically to skb_frag_page(), if the returning NULL is to catch its misuse
   for devmem, then I am agreed with this generally. But the NULL returning
   handling in kcm_write_msgs() seems to suggest otherwise to me. Isn't it
   reasonable to make the semantic obvious by using WARN_ON or BUG_ON directly in
   skb_frag_page(), and returning NULL does not 100% reliably crash the thread as
   suggested by jason?

But perhaps best is to show with data. Is there a representative page
pool benchmark that exercises the most sensitive case (XDP_DROP?) that
we can run with and without a static branch to demonstrate that any
diff is within the noise?

quoted

quoted

But none of this is related to correctness. Code calling
skb_frag_page() will fail or crash if it's not handled correctly
regardless of the implementation details of skb_frag_page(). In the
devmem series we add support to handle it correctly via [1] & [2].

--
Thanks,
Mina

-- 
Thanks,
Mina

.