RE: packet: Improve exception handling in fanout_add()

[David Laight]

From: Willem de Bruijn
> Sent: 01 January 2024 15:29
> 
> Markus Elfring wrote:
> > > It is fine to call kfree with a possible NULL pointer:
> > …
> > > 	 * If @object is NULL, no operation is performed.
> > > 	 */
> > > 	void kfree(const void *object)
> >
> > Such a function call triggers an input parameter validation
> > with a corresponding immediate return, doesn't it?
> > Do you find such data processing really helpful for the desired error/exception handling?
> 
> It's not just personal preference. It is an established pattern to
> avoid extra NULL tests around kfree.
> 
> A quick git log to show a few recent examples of patches that expressly
> remove such branches, e.g.,
> 
> commit d0110443cf4a ("amd/pds_core: core: No need for Null pointer check before kfree")
> commit efd9d065de67 ("drm/radeon: Remove unnecessary NULL test before kfree in
> 'radeon_connector_free_edid'")
> 
> An interesting older thread on the topic:
> 
> https://linux-kernel.vger.kernel.narkive.com/KVjlDsTo/kfree-null

That actually fails to note that if the call site contains the
conditional (explicitly or from a #define/static inline) then gcc
will optimise the test away if it can determine that the pointer
is NULL (from an earlier test) or non-NULL (has been dereferenced).
Possibly because gcc didn't do that 18 years ago.

> My summary, the many branches scattered throughout the kernel likely
> are more expensive than the occasional save from seeing the rare NULL
> pointer.

Especially in error paths or where the normal case is that the pointer
is allocated.

About the only place where a check in the caller may make sense is
for frequently used code paths where the pointer is normally NULL.
One example would be the code that reads an iovec[] from user space.
An on-stack array is used for small (sane) fragment counts with
kmalloc() being called for large counts.
In that case having 'if (unlikely(ptr)) kfree(ptr);' will probably
generate code that is measurably faster.
(Especially if there are mitigations for 'ret'.)

I also believe that likely/unlikely have almost no effect on modern x86.
(was it the P-Pro that used prefix for static prediction?)
IIRC there is no 'static prediction' - prediction is based on whatever
'set of branches' the current branch alases to.
The only slight gain is that the 'fall through' path is less likely
to stall due to a cache miss. But even that can be far enough ahead
of the non-speculative execution point that the actual stall is small.

Of course other simpler cpu do have static prediction rules.
Common would be to predict backwards branches taken (eg loops)
and forwards ones not-taken.
If you really do care (especially if you've disabled any dynamic prediction
in order to get measurable execution times) then you can need to add
(eg) asm comments to force a predicted not-taken forwards branch to
an unconditional backwards branch to avoid a 'predicted taken' backward
branch.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)