Re: [RFC Patch 1/10] Introducing generic hardware breakpoint handler interfaces

K.Prasad
2009-02-03T17:24:11Z

On Sun, Feb 01, 2009 at 01:05:35PM -0500, Alan Stern wrote:
> On Sun, 1 Feb 2009, Paul E. McKenney wrote:
>
> > > > Yes, indeed. With the current implementation, there's a possibility of
> > > > two instances of update_this_cpu() function executing - one with an
> > > > rcu_read_lock() taken (when called from load_debug_registers) while the
> > > > other without (when invoked through update_all_cpus()).
> > >
> > > No, this isn't possible unless I have misunderstood the nature of
> > > IPIs. Isn't is true that calling local_irq_save() will block delivery
> > > of IPIs?
> >
> > Touche! ;-)
> >
> > But in that case, why do you need the synchronize_rcu() following the
> > on_each_cpu() above? Is this needed to make sure that any concurrent
> > load_debug_registers() call has completed?
>
> No; it's needed to make sure that any concurrent
> switch_to_thread_hw_breakpoint() call has completed. That's where the
> important RCU read lock is taken. The routine is called not just by
> update_this_cpu() (and indirectly by load_debug_registers()) but also
> by __register_user_hw_breakpoint(), __unregister_user_hw_breakpoint(),
> and the task-switch routine.
>
> It's possible that the IPI from on_each_cpu() could interrupt an
> instance of switch_to_thread_hw_breakpoint() -- thereby causing it to
> run recursively. After the inner instance returns and the IPI is over,
> the outer instance will realize what has happened and restart itself.
> synchronize_rcu() insures that update_all_cpus() will wait until the
> outer instance is done.
>
> In fact, the RCU read lock in load_debug_registers() probably isn't
> necessary. But it's cleaner to leave it in; it points out that the
> routine accesses data structures which are protected by RCU.
>
> Alan Stern
>

Hi Alan,
After a better understanding about RCU usage in this patch, I'm
thinking if the list traversals in kernel/hw_breakpoint.c should be
changed into their RCU equivalent i.e. list_for_each_entry_rcu() instead
of list_for_each_entry() and list_del_rcu() instead of list_del() -
given that we are considering the list of thread HW breakpoints
(thread_bps) and kernel breakpoints (cur_kbpdata) which are also
accessed from exception-handler contexts.

I think that with the possibility of parallel execution of
the 'update' sections which would alter the protected data structures
(mentioned above) through functions such as - say insert_bp_in_list(),
balance_kernel_vs_user() and various other routines and the read-side
critical regions which need to be identified after converting the list
traversal routines it would be necessary to wrap the code around them
with rcu_read_(un)lock() routines.

What do you think? Is there something grossly incorrect in this
assessment of locking requirements?

Thanks,
K.Prasad