Hi Robert, I'm back in the office now; I just submitted an updated patch series to address some of the points you made below. I'll add responses in-line:
> -----Original Message----- > From: Sanford, Robert [mailto:rsanf...@akamai.com] > Sent: Wednesday, March 20, 2019 8:53 AM > To: Carrillo, Erik G <erik.g.carri...@intel.com>; tho...@monjalon.net; > dev@dpdk.org > Cc: nhor...@tuxdriver.com > Subject: Re: [PATCH v4 1/2] timer: allow timer management in shared > memory > > Hi Erik, > > I have a few questions and comments on this patch series. > > 1. Don't you think we need new tests (in test/test/) to verify the secondary- > process APIs? Yes, good idea. I'll work on a separate patch to add this. > 2. I suggest we define default_data_id as const, and explicitly set it to 0. I did change this to const, but ommitted the explicit initialization because checkpatch complains with the following: "ERROR:INITIALISED_STATIC: do not initialise statics to 0". > 3. The outer for-loop in rte_timer_alt_manage() touches beyond the end of > poll_lcores[]. I suggest a change like this: > > - for (i = 0, poll_lcore = poll_lcores[i]; i < nb_poll_lcores; > - poll_lcore = poll_lcores[++i]) { > + for (i = 0; I < nb_poll_lcores; i++) { > + poll_lcore = poll_lcores[i]; > Change made. > 4. Same problem (as #3) in the for-loop in rte_timer_stop_all(), in patch v4 > 2/2. Change made. > 5. There seems to be no difference between "typedef void > (*rte_timer_cb_t)(struct rte_timer *, void *)" and "typedef void > (*rte_timer_stop_all_cb_t)(struct rte_timer *tim, void *arg)", why add > rte_timer_stop_all_cb_t? Though they have the same signature, it seemed clearer to me to have a new callback type since one represents a function that gets called per timer, and the other represents a function that gets called for all timers. > 6. Can you provide a use case or code snippet that shows how we will use > rte_timer_alt_manage()? Currently this function is used by an updated version of the software event timer adapter (http://patchwork.dpdk.org/patch/48944/); rte_timer_alt_manage() is called in the service function for an instance of the adapter. Since this function allows timer_data_ids to be specified, different instances of the adapter can manage their own separate timer lists independently. > 7. Why not make the argument to rte_timer_alt_manage_cb_t a "struct > rte_timer *", instead of a "void *", since we pass a pointer-to-timer when we > invoke the function? > Change made. > -- > Regards, > Robert Sanford > Thanks, Erik
