netdev - Re: [PATCH 1/6] net/rds: Avoid queuing superfluous send and recv work

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7f47bb1a98a1d7cb026cf14b4da3fe761d33d46c.camel@oracle.com>
Date: Wed, 2 Apr 2025 01:34:40 +0000
From: Allison Henderson <allison.henderson@...cle.com>
To: "kuba@...nel.org" <kuba@...nel.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH 1/6] net/rds: Avoid queuing superfluous send and recv work

On Wed, 2025-03-26 at 09:42 -0700, Jakub Kicinski wrote:
> On Wed, 12 Mar 2025 07:50:11 +0000 Allison Henderson wrote:
> > Thread A:					Thread B:
> > -----------------------------------		-----------------------------------
> > 						Calls rds_sendmsg()
> > 						   Calls rds_send_xmit()
> > 						      Calls rds_cond_queue_send_work()   
> > Calls rds_send_worker()					
> >   calls rds_clear_queued_send_work_bit()		   
> >     clears RDS_SEND_WORK_QUEUED in cp->cp_flags		
> >       						         checks RDS_SEND_WORK_QUEUED in cp->cp_flags
> 
> But if the two threads run in parallel what prevents this check 
> to happen fully before the previous line on the "Thread A" side?
> 
> Please take a look at netif_txq_try_stop() for an example of 
> a memory-barrier based algo.
> 
> >       						         Queues work on on cp->cp_send_w
> >     Calls rds_send_xmit()
> >        Calls rds_cond_queue_send_work()
> >           skips queueing work on cp->cp_send_w

Hi Jakub,

I had a look at the example, how about we move the barriers from rds_clear_queued_send_work_bit into
rds_cond_queue_send_work?  Then we have something like this:

static inline void rds_cond_queue_send_work(struct rds_conn_path *cp, unsigned long delay)
{
	/* Ensure prior clear_bit operations for RDS_SEND_WORK_QUEUED are observed  */
	smp_mb__before_atomic();

        if (!test_and_set_bit(RDS_SEND_WORK_QUEUED, &cp->cp_flags))
                queue_delayed_work(rds_wq, &cp->cp_send_w, delay);

	/* Ensure the RDS_SEND_WORK_QUEUED bit is observed before proceeding */
	smp_mb__after_atomic();
}

I think that's more like whats in the example, and in line with what this patch is trying to do.  Let me know what you
think.

Thank you for the reviews!
Allison