Re: [RESEND PATCH] fs/pipe: Introduce a check to skip sleeping processes during pipe read/write

[WangYuli <wangyuli@xxxxxxxxxxxxx>]

On 2024/12/25 21:30, Andy Shevchenko wrote:

> Don't you think the Cc list is a bit overloaded?

Hi,

I apologize for any inconvenience this may cause.

I understand that under normal circumstances, one would simply pass the 
modified code path as an argument to the kernel's 
scripts/get_maintainer.pl script to determine the appropriate recipients.

However, given the vast and complex nature of the Linux kernel 
community, with tens of thousands of developers worldwide, and 
considering the varying "customs" of different subsystems, as well as 
time zone differences and individual work habits, it's not uncommon for 
patches to be sent to mailing lists and subsequently ignored or left 
pending.

This patch, for example, has been submitted multiple times without 
receiving any response, unfortunately.

My intention is simply to seek your review, and that of other technical 
experts like yourself, but I cannot be certain, prior to your response, 
which specific experts on which lists would be willing to provide feedback.

I would appreciate any other suggestions you may have.

> > UnixBench Pipe benchmark results on Zhaoxin KX-U6780A processor:
> >
> > With the option disabled: Single-core: 841.8, Multi-core (8): 4621.6
> > With the option enabled:  Single-core: 877.8, Multi-core (8): 4854.7
> >
> > Single-core performance improved by 4.1%, multi-core performance
> > improved by 4.8%.
> ...

As you know, the kernel is extremely sensitive to performance.

Even a 1% performance improvement can lead to significant efficiency 
gains and reduced carbon emissions in production environments, as long 
as there is sufficient testing and theoretical analysis to prove that 
the improvement is real and not due to measurement error or jitter.

> > +config PIPE_SKIP_SLEEPER
> > +	bool "Skip sleeping processes during pipe read/write"
> > +	default n
> 'n' is the default 'default', no need to have this line.
OK, I'll drop it. Thanks.
> > +	help
> > +	  This option introduces a check whether the sleep queue will
> > +	  be awakened during pipe read/write.
> > +
> > +	  It often leads to a performance improvement. However, in
> > +	  low-load or single-task scenarios, it may introduce minor
> > +	  performance overhead.
> > +	  If unsure, say N.
> Illogical, it's already N as you stated by putting a redundant line, but after
> removing that line it will make sense.
>
> ...
As noted, I'll remove "default n" as it serves no purpose.
> > +static inline bool
> Have you build this with Clang and `make W=1 ...`?

Hmm...I've noticed a discrepancy in kernel compilation results with and 
without "make W=1".

When I use x86_64_defconfig and clang-19.1.1 (Ubuntu 24.10) and run 
"make", there are no warnings.

However, when I run "make W=1", the kernel generates a massive number of 
errors, causing the compilation to fail prematurely.

e.g.

In file included from arch/x86/kernel/asm-offsets.c:14:
In file included from ./include/linux/suspend.h:5:
In file included from ./include/linux/swap.h:9:
In file included from ./include/linux/memcontrol.h:21:
In file included from ./include/linux/mm.h:2224:
./include/linux/vmstat.h:504:43: error: arithmetic between different 
enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') 
[-Werror,-Wenum-enum-conversion]
  504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
      |                            ~~~~~~~~~~~~~~~~~~~~~ ^
  505 |                            item];
      |                            ~~~~
./include/linux/vmstat.h:511:43: error: arithmetic between different 
enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') 
[-Werror,-Wenum-enum-conversion]
  511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
      |                            ~~~~~~~~~~~~~~~~~~~~~ ^
  512 |                            NR_VM_NUMA_EVENT_ITEMS +
      |                            ~~~~~~~~~~~~~~~~~~~~~~
./include/linux/vmstat.h:524:43: error: arithmetic between different 
enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') 
[-Werror,-Wenum-enum-conversion]
  524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
      |                            ~~~~~~~~~~~~~~~~~~~~~ ^
  525 |                            NR_VM_NUMA_EVENT_ITEMS +
      |                            ~~~~~~~~~~~~~~~~~~~~~~
3 errors generated.

And I've observed similar behavior with gcc-14.2.0.

While I'm keen on addressing as many potential compile errors and 
warnings in the kernel as possible, it seems like a long-term endeavor.

Regarding this specific code, I'd appreciate your insights on how to 
improve it.

> > +pipe_check_wq_has_sleeper(struct wait_queue_head *wq_head)
> > +{
> > +	if (IS_ENABLED(CONFIG_PIPE_SKIP_SLEEPER))
> > +		return wq_has_sleeper(wq_head);
> > +	else
> Redundant.
>
> > +		return true;
> 	if (!foo)
> 		return true;
>
> 	return bar(...);
>
> > +}

Yes. I'll rework the code structure here. Thanks.

Thanks,
--
WangYuli

Attachment: OpenPGP_0xC5DA1F3046F40BEE.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature