[dpdk-dev] [PATCH v4 1/2] timer: allow timer management in shared memory

[Carrillo, Erik G]

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:rsanford at akamai.com]
> Sent: Wednesday, March 20, 2019 8:53 AM
> To: Carrillo, Erik G <erik.g.carrillo at intel.com>; thomas at monjalon.net;
> dev at dpdk.org
> Cc: nhorman at 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