Re: 回复: [PATCH] workqueue: Make a warning when a pending delay work being re-init

[Thomas Weißschuh]

On 2023-11-16 01:53:30+0000, 黄再漾(Joyyoung Huang) wrote:
> 
> On 2023-11-15 19:34:27+0800, huangzaiyang wrote:
> > There is a timer_list race condition if a delay work is queued twice,
> > this usually won't happen unless someone reinitializes the task before performing the enqueue operation,likeï¼?
> > https://github.com/torvalds/linux/blob/master/drivers/devfreq/devfreq.
> > c#L487 A warning message will help developers identify this irregular
> > usage.
> >
> > Signed-off-by: ZaiYang Huang <hzy0719@xxxxxxx>
> > ---
> >  include/linux/workqueue.h | 33 ++++++++++++++++++---------------
> >  1 file changed, 18 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> > index 24b1e5070f4d..54102ed794e5 100644
> > --- a/include/linux/workqueue.h
> > +++ b/include/linux/workqueue.h
> > @@ -266,6 +266,22 @@ static inline void
> > destroy_delayed_work_on_stack(struct delayed_work *work) { }  static
> > inline unsigned int work_static(struct work_struct *work) { return 0;
> > }  #endif
> >
> > +/**
> > + * work_pending - Find out whether a work item is currently pending
> > + * @work: The work item in question
> > + */
> > +#define work_pending(work) \
> > +       test_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(work))
> > +
> > +/**
> > + * delayed_work_pending - Find out whether a delayable work item is
> > +currently
> > + * pending
> > + * @w: The work item in question
> > + */
> > +#define delayed_work_pending(w) \
> > +       work_pending(&(w)->work)
> > +
> > +
> >  /*
> >   * initialize all of a work item in one go
> >   *
> > @@ -310,6 +326,7 @@ static inline unsigned int work_static(struct
> > work_struct *work) { return 0; }
> >
> >  #define __INIT_DELAYED_WORK(_work, _func, _tflags)                     \
> >         do {                                                            \
> > +               WARN_ON(delayed_work_pending(_work));                   \
> 
> How does this work when the data _work points to is not yet initialized?
> Reading uninitialized data is UB and may spuriously trigger the warning.
> 
> --> The data member in work_struct is a variable of type atomic_long_t, and 0 is the initial value of the atomic variable, right?

Please use proper email quoting style, using "> ".

Normal integers are not initialized by default when allocated on the
stack. I don't think atomic_long_t behaves differently.

> --> On the other hand, if a variable of type work_struct is dynamically allocated, an unnecessary warning may be generated randomly; however, it should be a good habit to initialize dynamically allocated variables to 0.

It seems surprising having to initialize a variable to 0 only to
directly afterwards initialize it again with INIT_DELAYED_WORK().

Also even if there was consensus that it should be done this way,
existing users of this macro don't do it, so these would need to be
adapted first.

> >                 INIT_WORK(&(_work)->work, (_func));                     \
> >                 __init_timer(&(_work)->timer,                           \
> >                              delayed_work_timer_fn,                     \
> > @@ -318,6 +335,7 @@ static inline unsigned int work_static(struct
> > work_struct *work) { return 0; }
> 
> [..]
> 
> > --
> > 2.17.1
> >
> > ________________________________
> > OPPO
> 
> The gunk at the end of the mail will prevent your patch from being considered at all.
> 
> >
> > ±¾µç×ÓÓʼþ¼°Æ丽¼þº¬ÓÐOPPO¹«Ë¾µÄ±£ÃÜÐÅÏ¢£¬½öÏÞÓÚÓʼþÖ¸Ã÷µÄÊÕ¼þÈË£¨°üº¬
> > ¸öÈ˼°Èº×飩ʹÓ᣽ûÖ¹ÈκÎÈËÔÚδ¾­ÊÚȨµÄÇé¿öÏÂÒÔÈκÎÐÎʽʹÓá£Èç¹ûÄú´í
> > ÊÕÁ˱¾Óʼþ£¬ÇÐÎð´«²¥¡¢·Ö·¢¡¢¸´ÖÆ¡¢Ó¡Ë¢»òʹÓñ¾ÓʼþÖ®Èκβ¿·Ö»òÆäËùÔØÖ®
> > ÈκÎÄÚÈÝ£¬²¢ÇëÁ¢¼´ÒÔµç×ÓÓʼþ֪ͨ·¢¼þÈ˲¢É¾³ý±¾Óʼþ¼°Æ丽¼þ¡£

This stuff really needs to go.