[GitHub] [incubator-nuttx] GUIDINGLI commented on a diff in pull request #7018: wqueue: change single queue to double queue to improve speed

[GitBox <gi...@apache.org>]

GUIDINGLI commented on code in PR #7018:
URL: https://github.com/apache/incubator-nuttx/pull/7018#discussion_r964548591


##########
include/nuttx/wdog.h:
##########
@@ -61,17 +61,22 @@ typedef uint32_t  wdparm_t;
 
 typedef CODE void (*wdentry_t)(wdparm_t arg);
 
-/* This is the internal representation of the watchdog timer structure. */
+/* This is the internal representation of the watchdog timer structure.
+ * Notice !!!
+ * Carefully with the struct wdog_s order, you may not directly modify
+ * this. This struct will combine in struct work_s in union type, and,
+ * wqueue will modify/check this struct in work_qcancel().
+ */
 
 struct wdog_s
 {
   FAR struct wdog_s *next;       /* Support for singly linked lists. */
+  wdparm_t           arg;        /* Callback argument */

Review Comment:
   in struct work_s:
   union
   {
     struct
     {
       struct dq_entry_s dq; /* Implements a double linked list */
       clock_t qtime;        /* Time work queued */
     } s;
     struct wdog_s timer;    /* Delay expiry timer */
   }
   
   while we use WDOG_ISACTIVE(&work->timer) to decide use dq or timer,
   that is error, wd_cancel() maybe modify this area, dq_rem() also can
   modify this area.
   So we can't use the WDOG_ISACTIVE(&work->timer) to take as the judgement,
   when there is a union.
   
   Fix:
   swap the order in struct wdog_s, move the arg to the second 32bit
   
   ```
   
    64 static int work_qcancel(FAR struct kwork_wqueue_s *wqueue,
    65                         FAR struct work_s *work)
    66 {
    67   irqstate_t flags;
    68   int ret = -ENOENT;
    69 
    70   DEBUGASSERT(work != NULL);
    71 
    72   /* Cancelling the work is simply a matter of removing the work structure
    73    * from the work queue.  This must be done with interrupts disabled because
    74    * new work is typically added to the work queue from interrupt handlers.
    75    */
    76 
    77   flags = enter_critical_section();
    78   if (work->worker != NULL)
    79     {
    80       /* Remove the entry from the work queue and make sure that it is
    81        * marked as available (i.e., the worker field is nullified).
    82        */
    83 
    84       if (WDOG_ISACTIVE(&work->u.timer))
    85         {
    86           wd_cancel(&work->u.timer);
    87         }
    88       else
    89         {
    90           dq_rem((FAR dq_entry_t *)work, &wqueue->q);
    91         }
    92 
    93       work->worker = NULL;
    94       ret = OK;
    95     }
    96 
    97   leave_critical_section(flags);
    98   return ret;
    99 }
   
   ```
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org