A few of my yesterday's replies made no sense... Lesson learned: don't
reply to code review comments on a late friday evening :)
On 23-Mar-18 6:21 PM, Burakov, Anatoly wrote:
On 23-Mar-18 3:38 PM, Tan, Jianfeng wrote:
We do. However, we have to wait for *something* if there aren't any
asynchronous requests pending. There isn't a way to put "wait infinite
amount" as a time value, so i opted for next best thing - large enough
to not cause any performance issues. The timeout is arbitrary.
Didn't realize we were holding the lock, so we could choose between wait
and timed wait. Fixed in v5.
/** Double linked list of actions. */
@@ -60,13 +65,37 @@ struct mp_msg_internal {
struct rte_mp_msg msg;
};
+enum mp_request_type {
+ REQUEST_TYPE_SYNC,
+ REQUEST_TYPE_ASYNC
+};
+
+struct async_request_shared_param {
+ struct rte_mp_reply *user_reply;
+ struct timespec *end;
Why we have to make these two as pointers? Operating on pointers
introduce unnecessary complexity.
Because those are shared between different pending requests. Each
pending request gets its own entry in the queue (because it expects
answer from a particular process), but the request data (callback,
number of requests processed, etc.) is shared between all requests for
this sync operation. We don't have the luxury of storing all of that in
a local variable like we do with synchronous requests :)
Missed the fact that you weren't referring to the need of storing these
in shared_param but rather to the fact that i was storing
malloc-allocated values that are shared, as pointers in shared param
structure, when i could just as easily store actual structs there. Fixed
in v5.
Too many structs are defined? How about just putting it like this:
struct pending_request {
TAILQ_ENTRY(sync_request) next;
enum mp_request_type type;
char dst[PATH_MAX];
struct rte_mp_msg *request;
struct rte_mp_msg *reply_msg;
int reply_received;
RTE_STD_C11
union {
/* for sync request */
struct {
pthread_cond_t cond; /* used for mp thread to
wake up requesting thread */
};
/* for async request */
struct {
struct rte_mp_reply user_reply;
struct timespec end;
int n_requests_processed; /* store how
requests */
};
};
};
That can work, sure. However, i actually think that my approach is
clearer, because when you're working with autocomplete and a proper IDE,
it's clear which values are for which case (and i would argue it makes
the code more readable as well).
Again, didn't realize that you were referring to defining all of the
structs and values outside of pending request, rather than storing them
as anonymous structs (which would indeed cause problems with
autocomplete and readability). Fixed in v5.
Thanks again for your review!
--
Thanks,
Anatoly
