linux-kernel - Re: [PATCH v3 tip/core/rcu 1/9] rcu: Add call_rcu

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]

Date:	Fri, 1 Aug 2014 11:28:37 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	linux-kernel@...r.kernel.org, mingo@...nel.org,
	laijs@...fujitsu.com, dipankar@...ibm.com,
	akpm@...ux-foundation.org, mathieu.desnoyers@...icios.com,
	josh@...htriplett.org, tglx@...utronix.de, peterz@...radead.org,
	rostedt@...dmis.org, dhowells@...hat.com, edumazet@...gle.com,
	dvhart@...ux.intel.com, fweisbec@...il.com, bobby.prani@...il.com
Subject: Re: [PATCH v3 tip/core/rcu 1/9] rcu: Add call_rcu_tasks()

On Fri, Aug 01, 2014 at 04:11:44PM +0200, Oleg Nesterov wrote:
> On 07/31, Paul E. McKenney wrote:
> >
> > +/* Lists of tasks that we are still waiting for during this grace period. */
> > +static LIST_HEAD(rcu_tasks_holdouts);

Good point, fixed!

> This can be local var in rcu_tasks_kthread()
> 
> > +		while (!list_empty(&rcu_tasks_holdouts)) {
> > +			schedule_timeout_interruptible(HZ / 10);
> > +			flush_signals(current);
> 
> Still can't undestand why your paranoia wants flush_signals ;)
> This is unneeded and confusing. If you think we can have a bug here,
> then we should ot hide it, WARN_ON(signal_pending) would be better.
> And if you think signal_pending(current) is possible, why do you
> check this only after schedule_interruptible() ?

I can live with WARN_ON(signal_pending(current)).  Fixed!

> > +		synchronize_sched();
> > +
> > +		/* Invoke the callbacks. */
> > +		while (list) {
> > +			next = list->next;
> > +			local_bh_disable();
> > +			list->func(list);
> > +			local_bh_enable();
> > +			list = next;
> > +			cond_resched();
> > +		}
> 
> Not sure this makes any sense, but perhaps we can check for the new
> callbacks and start the next gp. IOW, the main loop roughly does
> 
> 	for (;;) {
> 		list = rcu_tasks_cbs_head;
> 		rcu_tasks_cbs_head = NULL;
> 
> 		if (!list)
> 			sleep();
> 
> 		synchronize_sched();
> 
> 		wait_for_rcu_tasks_holdout();
> 
> 		synchronize_sched();
> 
> 		process_callbacks(list);
> 	}
> 
> we can "join" 2 synchronize_sched's and do
> 
> 	ready_list = NULL;
> 	for (;;) {
> 		list = rcu_tasks_cbs_head;
> 		rcu_tasks_cbs_head = NULL;
> 
> 		if (!list && !ready_list)
> 			sleep();
> 
> 		synchronize_sched();
> 
> 		if (ready_list) {
> 			process_callbacks(ready_list);
> 			ready_list = NULL;
> 		}
> 
> 		if (!list)
> 			continue;
> 
> 		wait_for_rcu_tasks_holdout();
> 		ready_list = list;
> 	}

The lack of barriers for the updates I am checking mean that I really
do need a synchronize_sched() on either side of the grace-period wait.
The grace period needs to guarantee that anything that happened on any
CPU before the start of the grace period happens before anything that
happens on any CPU after the end of the grace period.  If I leave off
either synchronize_sched(), we lose this guarantee.

							Thanx, Paul

> Oleg.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/