| Message ID | 20220125010458.2326473-1-luiz.dentz@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/2] shared/mgmt: Add request timeout handling | expand |
This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=608129 ---Test result--- Test Summary: CheckPatch PASS 2.97 seconds GitLint PASS 2.03 seconds Prep - Setup ELL PASS 42.90 seconds Build - Prep PASS 0.72 seconds Build - Configure PASS 8.71 seconds Build - Make PASS 1414.03 seconds Make Check PASS 11.53 seconds Make Check w/Valgrind PASS 446.01 seconds Make Distcheck PASS 233.35 seconds Build w/ext ELL - Configure PASS 8.72 seconds Build w/ext ELL - Make PASS 1423.32 seconds Incremental Build with patchesPASS 2825.31 seconds --- Regards, Linux Bluetooth
Hi Luiz, > This adds request timeout handling, by default the timeout is set to 2 > seconds but in case the user want a different timeout it can use > mgmt_send_timeout to set a different value. > --- > src/shared/mgmt.c | 70 +++++++++++++++++++++++++++++++++++++++++------ > src/shared/mgmt.h | 5 ++++ > 2 files changed, 67 insertions(+), 8 deletions(-) > > diff --git a/src/shared/mgmt.c b/src/shared/mgmt.c > index 41457b430..291a2c636 100644 > --- a/src/shared/mgmt.c > +++ b/src/shared/mgmt.c > @@ -25,6 +25,9 @@ > #include "src/shared/queue.h" > #include "src/shared/util.h" > #include "src/shared/mgmt.h" > +#include "src/shared/timeout.h" > + > +#define MGMT_TIMEOUT 2 > > struct mgmt { > int ref_count; > @@ -49,6 +52,7 @@ struct mgmt { > }; > > struct mgmt_request { > + struct mgmt *mgmt; > unsigned int id; > uint16_t opcode; > uint16_t index; > @@ -57,6 +61,8 @@ struct mgmt_request { > mgmt_request_func_t callback; > mgmt_destroy_func_t destroy; > void *user_data; > + int timeout; > + unsigned int timeout_id; > }; > > struct mgmt_notify { > @@ -81,6 +87,9 @@ static void destroy_request(void *data) > if (request->destroy) > request->destroy(request->user_data); > > + if (request->timeout_id) > + timeout_remove(request->timeout_id); > + > free(request->buf); > free(request); > } > @@ -150,6 +159,26 @@ static void write_watch_destroy(void *user_data) > mgmt->writer_active = false; > } > > +static bool request_timeout(void *data) > +{ > + struct mgmt_request *request = data; > + > + if (!request) > + return false; > + > + request->timeout_id = 0; > + > + queue_remove_if(request->mgmt->pending_list, NULL, request); > + > + if (request->callback) > + request->callback(MGMT_STATUS_TIMEOUT, 0, NULL, > + request->user_data); > + > + destroy_request(request); > + > + return false; > +} > + > static bool send_request(struct mgmt *mgmt, struct mgmt_request *request) > { > struct iovec iov; > @@ -169,6 +198,12 @@ static bool send_request(struct mgmt *mgmt, struct mgmt_request *request) > return false; > } > > + if (request->timeout) > + request->timeout_id = timeout_add_seconds(request->timeout, > + request_timeout, > + request, > + NULL); > + > util_debug(mgmt->debug_callback, mgmt->debug_data, > "[0x%04x] command 0x%04x", > request->index, request->opcode); > @@ -566,7 +601,8 @@ bool mgmt_set_close_on_unref(struct mgmt *mgmt, bool do_close) > static struct mgmt_request *create_request(struct mgmt *mgmt, uint16_t opcode, > uint16_t index, uint16_t length, > const void *param, mgmt_request_func_t callback, > - void *user_data, mgmt_destroy_func_t destroy) > + void *user_data, mgmt_destroy_func_t destroy, > + int timeout) > { > struct mgmt_request *request; > struct mgmt_hdr *hdr; > @@ -598,12 +634,18 @@ static struct mgmt_request *create_request(struct mgmt *mgmt, uint16_t opcode, > hdr->index = htobs(index); > hdr->len = htobs(length); > > + /* Use a weak reference so requests don't prevent mgmt_unref to > + * cancel requests and free in case of the last reference is dropped by > + * the user. > + */ > + request->mgmt = mgmt; > request->opcode = opcode; > request->index = index; > > request->callback = callback; > request->destroy = destroy; > request->user_data = user_data; > + request->timeout = timeout; > > return request; > } > @@ -735,10 +777,11 @@ unsigned int mgmt_send_tlv(struct mgmt *mgmt, uint16_t opcode, uint16_t index, > return ret; > } > > -unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index, > - uint16_t length, const void *param, > - mgmt_request_func_t callback, > - void *user_data, mgmt_destroy_func_t destroy) > +unsigned int mgmt_send_timeout(struct mgmt *mgmt, uint16_t opcode, > + uint16_t index, uint16_t length, > + const void *param, mgmt_request_func_t callback, > + void *user_data, mgmt_destroy_func_t destroy, > + int timeout) > { > struct mgmt_request *request; > > @@ -746,7 +789,7 @@ unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index, > return 0; > > request = create_request(mgmt, opcode, index, length, param, > - callback, user_data, destroy); > + callback, user_data, destroy, timeout); > if (!request) > return 0; > > @@ -766,6 +809,15 @@ unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index, > return request->id; > } > > +unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index, > + uint16_t length, const void *param, > + mgmt_request_func_t callback, > + void *user_data, mgmt_destroy_func_t destroy) > +{ > + return mgmt_send_timeout(mgmt, opcode, index, length, param, callback, > + user_data, destroy, MGMT_TIMEOUT); > +} > + I am not happy with this. No every command has to setup a timeout handler and so on. This is not really what should be done since the mgmt communication with the kernel is actually pretty low latency. If you want to add a special send_timeout, then do that, but leave the send alone. Regards Marcel
Hi Marcel, On Tue, Jan 25, 2022 at 1:38 PM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Luiz, > > > This adds request timeout handling, by default the timeout is set to 2 > > seconds but in case the user want a different timeout it can use > > mgmt_send_timeout to set a different value. > > --- > > src/shared/mgmt.c | 70 +++++++++++++++++++++++++++++++++++++++++------ > > src/shared/mgmt.h | 5 ++++ > > 2 files changed, 67 insertions(+), 8 deletions(-) > > > > diff --git a/src/shared/mgmt.c b/src/shared/mgmt.c > > index 41457b430..291a2c636 100644 > > --- a/src/shared/mgmt.c > > +++ b/src/shared/mgmt.c > > @@ -25,6 +25,9 @@ > > #include "src/shared/queue.h" > > #include "src/shared/util.h" > > #include "src/shared/mgmt.h" > > +#include "src/shared/timeout.h" > > + > > +#define MGMT_TIMEOUT 2 > > > > struct mgmt { > > int ref_count; > > @@ -49,6 +52,7 @@ struct mgmt { > > }; > > > > struct mgmt_request { > > + struct mgmt *mgmt; > > unsigned int id; > > uint16_t opcode; > > uint16_t index; > > @@ -57,6 +61,8 @@ struct mgmt_request { > > mgmt_request_func_t callback; > > mgmt_destroy_func_t destroy; > > void *user_data; > > + int timeout; > > + unsigned int timeout_id; > > }; > > > > struct mgmt_notify { > > @@ -81,6 +87,9 @@ static void destroy_request(void *data) > > if (request->destroy) > > request->destroy(request->user_data); > > > > + if (request->timeout_id) > > + timeout_remove(request->timeout_id); > > + > > free(request->buf); > > free(request); > > } > > @@ -150,6 +159,26 @@ static void write_watch_destroy(void *user_data) > > mgmt->writer_active = false; > > } > > > > +static bool request_timeout(void *data) > > +{ > > + struct mgmt_request *request = data; > > + > > + if (!request) > > + return false; > > + > > + request->timeout_id = 0; > > + > > + queue_remove_if(request->mgmt->pending_list, NULL, request); > > + > > + if (request->callback) > > + request->callback(MGMT_STATUS_TIMEOUT, 0, NULL, > > + request->user_data); > > + > > + destroy_request(request); > > + > > + return false; > > +} > > + > > static bool send_request(struct mgmt *mgmt, struct mgmt_request *request) > > { > > struct iovec iov; > > @@ -169,6 +198,12 @@ static bool send_request(struct mgmt *mgmt, struct mgmt_request *request) > > return false; > > } > > > > + if (request->timeout) > > + request->timeout_id = timeout_add_seconds(request->timeout, > > + request_timeout, > > + request, > > + NULL); > > + > > util_debug(mgmt->debug_callback, mgmt->debug_data, > > "[0x%04x] command 0x%04x", > > request->index, request->opcode); > > @@ -566,7 +601,8 @@ bool mgmt_set_close_on_unref(struct mgmt *mgmt, bool do_close) > > static struct mgmt_request *create_request(struct mgmt *mgmt, uint16_t opcode, > > uint16_t index, uint16_t length, > > const void *param, mgmt_request_func_t callback, > > - void *user_data, mgmt_destroy_func_t destroy) > > + void *user_data, mgmt_destroy_func_t destroy, > > + int timeout) > > { > > struct mgmt_request *request; > > struct mgmt_hdr *hdr; > > @@ -598,12 +634,18 @@ static struct mgmt_request *create_request(struct mgmt *mgmt, uint16_t opcode, > > hdr->index = htobs(index); > > hdr->len = htobs(length); > > > > + /* Use a weak reference so requests don't prevent mgmt_unref to > > + * cancel requests and free in case of the last reference is dropped by > > + * the user. > > + */ > > + request->mgmt = mgmt; > > request->opcode = opcode; > > request->index = index; > > > > request->callback = callback; > > request->destroy = destroy; > > request->user_data = user_data; > > + request->timeout = timeout; > > > > return request; > > } > > @@ -735,10 +777,11 @@ unsigned int mgmt_send_tlv(struct mgmt *mgmt, uint16_t opcode, uint16_t index, > > return ret; > > } > > > > -unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index, > > - uint16_t length, const void *param, > > - mgmt_request_func_t callback, > > - void *user_data, mgmt_destroy_func_t destroy) > > +unsigned int mgmt_send_timeout(struct mgmt *mgmt, uint16_t opcode, > > + uint16_t index, uint16_t length, > > + const void *param, mgmt_request_func_t callback, > > + void *user_data, mgmt_destroy_func_t destroy, > > + int timeout) > > { > > struct mgmt_request *request; > > > > @@ -746,7 +789,7 @@ unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index, > > return 0; > > > > request = create_request(mgmt, opcode, index, length, param, > > - callback, user_data, destroy); > > + callback, user_data, destroy, timeout); > > if (!request) > > return 0; > > > > @@ -766,6 +809,15 @@ unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index, > > return request->id; > > } > > > > +unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index, > > + uint16_t length, const void *param, > > + mgmt_request_func_t callback, > > + void *user_data, mgmt_destroy_func_t destroy) > > +{ > > + return mgmt_send_timeout(mgmt, opcode, index, length, param, callback, > > + user_data, destroy, MGMT_TIMEOUT); > > +} > > + > > I am not happy with this. No every command has to setup a timeout handler and so on. This is not really what should be done since the mgmt communication with the kernel is actually pretty low latency. Well we don't have any way to check if the kernel is really responding within a reasonable time, so I thought it would be a good idea to have a default to capture when a command /reply does not respond, otherwise it is pretty hard to debug such conditions since the request will stay in the queue indefinitely. > If you want to add a special send_timeout, then do that, but leave the send alone. > > Regards > > Marcel >
diff --git a/src/shared/mgmt.c b/src/shared/mgmt.c index 41457b430..291a2c636 100644 --- a/src/shared/mgmt.c +++ b/src/shared/mgmt.c @@ -25,6 +25,9 @@ #include "src/shared/queue.h" #include "src/shared/util.h" #include "src/shared/mgmt.h" +#include "src/shared/timeout.h" + +#define MGMT_TIMEOUT 2 struct mgmt { int ref_count; @@ -49,6 +52,7 @@ struct mgmt { }; struct mgmt_request { + struct mgmt *mgmt; unsigned int id; uint16_t opcode; uint16_t index; @@ -57,6 +61,8 @@ struct mgmt_request { mgmt_request_func_t callback; mgmt_destroy_func_t destroy; void *user_data; + int timeout; + unsigned int timeout_id; }; struct mgmt_notify { @@ -81,6 +87,9 @@ static void destroy_request(void *data) if (request->destroy) request->destroy(request->user_data); + if (request->timeout_id) + timeout_remove(request->timeout_id); + free(request->buf); free(request); } @@ -150,6 +159,26 @@ static void write_watch_destroy(void *user_data) mgmt->writer_active = false; } +static bool request_timeout(void *data) +{ + struct mgmt_request *request = data; + + if (!request) + return false; + + request->timeout_id = 0; + + queue_remove_if(request->mgmt->pending_list, NULL, request); + + if (request->callback) + request->callback(MGMT_STATUS_TIMEOUT, 0, NULL, + request->user_data); + + destroy_request(request); + + return false; +} + static bool send_request(struct mgmt *mgmt, struct mgmt_request *request) { struct iovec iov; @@ -169,6 +198,12 @@ static bool send_request(struct mgmt *mgmt, struct mgmt_request *request) return false; } + if (request->timeout) + request->timeout_id = timeout_add_seconds(request->timeout, + request_timeout, + request, + NULL); + util_debug(mgmt->debug_callback, mgmt->debug_data, "[0x%04x] command 0x%04x", request->index, request->opcode); @@ -566,7 +601,8 @@ bool mgmt_set_close_on_unref(struct mgmt *mgmt, bool do_close) static struct mgmt_request *create_request(struct mgmt *mgmt, uint16_t opcode, uint16_t index, uint16_t length, const void *param, mgmt_request_func_t callback, - void *user_data, mgmt_destroy_func_t destroy) + void *user_data, mgmt_destroy_func_t destroy, + int timeout) { struct mgmt_request *request; struct mgmt_hdr *hdr; @@ -598,12 +634,18 @@ static struct mgmt_request *create_request(struct mgmt *mgmt, uint16_t opcode, hdr->index = htobs(index); hdr->len = htobs(length); + /* Use a weak reference so requests don't prevent mgmt_unref to + * cancel requests and free in case of the last reference is dropped by + * the user. + */ + request->mgmt = mgmt; request->opcode = opcode; request->index = index; request->callback = callback; request->destroy = destroy; request->user_data = user_data; + request->timeout = timeout; return request; } @@ -735,10 +777,11 @@ unsigned int mgmt_send_tlv(struct mgmt *mgmt, uint16_t opcode, uint16_t index, return ret; } -unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index, - uint16_t length, const void *param, - mgmt_request_func_t callback, - void *user_data, mgmt_destroy_func_t destroy) +unsigned int mgmt_send_timeout(struct mgmt *mgmt, uint16_t opcode, + uint16_t index, uint16_t length, + const void *param, mgmt_request_func_t callback, + void *user_data, mgmt_destroy_func_t destroy, + int timeout) { struct mgmt_request *request; @@ -746,7 +789,7 @@ unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index, return 0; request = create_request(mgmt, opcode, index, length, param, - callback, user_data, destroy); + callback, user_data, destroy, timeout); if (!request) return 0; @@ -766,6 +809,15 @@ unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index, return request->id; } +unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index, + uint16_t length, const void *param, + mgmt_request_func_t callback, + void *user_data, mgmt_destroy_func_t destroy) +{ + return mgmt_send_timeout(mgmt, opcode, index, length, param, callback, + user_data, destroy, MGMT_TIMEOUT); +} + unsigned int mgmt_send_nowait(struct mgmt *mgmt, uint16_t opcode, uint16_t index, uint16_t length, const void *param, mgmt_request_func_t callback, @@ -777,7 +829,8 @@ unsigned int mgmt_send_nowait(struct mgmt *mgmt, uint16_t opcode, uint16_t index return 0; request = create_request(mgmt, opcode, index, length, param, - callback, user_data, destroy); + callback, user_data, destroy, + MGMT_TIMEOUT); if (!request) return 0; @@ -803,7 +856,8 @@ unsigned int mgmt_reply(struct mgmt *mgmt, uint16_t opcode, uint16_t index, return 0; request = create_request(mgmt, opcode, index, length, param, - callback, user_data, destroy); + callback, user_data, destroy, + MGMT_TIMEOUT); if (!request) return 0; diff --git a/src/shared/mgmt.h b/src/shared/mgmt.h index 56add613d..e4f1a7934 100644 --- a/src/shared/mgmt.h +++ b/src/shared/mgmt.h @@ -55,6 +55,11 @@ unsigned int mgmt_send(struct mgmt *mgmt, uint16_t opcode, uint16_t index, uint16_t length, const void *param, mgmt_request_func_t callback, void *user_data, mgmt_destroy_func_t destroy); +unsigned int mgmt_send_timeout(struct mgmt *mgmt, uint16_t opcode, + uint16_t index, uint16_t length, + const void *param, mgmt_request_func_t callback, + void *user_data, mgmt_destroy_func_t destroy, + int timeout); unsigned int mgmt_send_nowait(struct mgmt *mgmt, uint16_t opcode, uint16_t index, uint16_t length, const void *param, mgmt_request_func_t callback,
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> This adds request timeout handling, by default the timeout is set to 2 seconds but in case the user want a different timeout it can use mgmt_send_timeout to set a different value. --- src/shared/mgmt.c | 70 +++++++++++++++++++++++++++++++++++++++++------ src/shared/mgmt.h | 5 ++++ 2 files changed, 67 insertions(+), 8 deletions(-)