Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not start SCTP iterator thread when library is initialized via usrsctp_init_nothreads. #472

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mstyura
Copy link
Contributor

@mstyura mstyura commented May 22, 2020

I believe it make sense not to stat SCTP iterator thread, when library was intentionally initialized via usrsctp_init_nothreads.

In case user of library desire to disable threads there should no threads created.

I see main use cases of usrsctp_init_nothreads is to implement single threaded processing of SCTP packets for applications which need to implement WebRTC's data channel.

It is occurred that completely single threaded processing is impossible due to SCTP iterator is still started when library initialized via usrsctp_init_nothreads.

Moreover this thread is still doing some work - it is not always in dormant state.
So far I've figured out that some processing on SCTP iterator is triggered by timer, with example call stack:

sctp_wakeup_iterator() usrsctplib/netinet/sctp_bsd_addr.c:114)
sctp_initiate_iterator(inp_func inpf, asoc_func af, inp_func inpe, uint32_t pcb_state, uint32_t pcb_features, uint32_t asoc_state, void * argp, uint32_t argi, end_func ef, struct sctp_inpcb * s_inp, uint8_t chunk_output_off) usrsctplib/netinet/sctp_pcb.c:8207)
sctp_handle_addr_wq() usrsctplib/netinet/sctputil.c:1725)
sctp_timeout_handler(void * t) usrsctplib/netinet/sctputil.c:2175)
sctp_handle_tick(uint32_t elapsed_ticks) usrsctplib/netinet/sctp_callout.c:172)
usrsctp_handle_timers(uint32_t delta) usrsctplib/user_socket.c:3548)

case SCTP_TIMER_TYPE_ADDR_WQ:
KASSERT(inp == NULL && stcb == NULL && net == NULL,
("timeout of type %d: inp = %p, stcb = %p, net = %p",
type, inp, stcb, net));
sctp_handle_addr_wq();
break;

static void
sctp_handle_addr_wq(void)
{
/* deal with the ADDR wq from the rtsock calls */
struct sctp_laddr *wi, *nwi;
struct sctp_asconf_iterator *asc;
SCTP_MALLOC(asc, struct sctp_asconf_iterator *,
sizeof(struct sctp_asconf_iterator), SCTP_M_ASC_IT);
if (asc == NULL) {
/* Try later, no memory */
sctp_timer_start(SCTP_TIMER_TYPE_ADDR_WQ,
(struct sctp_inpcb *)NULL,
(struct sctp_tcb *)NULL,
(struct sctp_nets *)NULL);
return;
}
LIST_INIT(&asc->list_of_work);
asc->cnt = 0;
LIST_FOREACH_SAFE(wi, &SCTP_BASE_INFO(addr_wq), sctp_nxt_addr, nwi) {
LIST_REMOVE(wi, sctp_nxt_addr);
LIST_INSERT_HEAD(&asc->list_of_work, wi, sctp_nxt_addr);
asc->cnt++;
}
if (asc->cnt == 0) {
SCTP_FREE(asc, SCTP_M_ASC_IT);
} else {
int ret;
ret = sctp_initiate_iterator(sctp_asconf_iterator_ep,
sctp_asconf_iterator_stcb,
NULL, /* No ep end for boundall */
SCTP_PCB_FLAGS_BOUNDALL,
SCTP_PCB_ANY_FEATURES,
SCTP_ASOC_ANY_STATE,
(void *)asc, 0,
sctp_asconf_iterator_end, NULL, 0);
if (ret) {
SCTP_PRINTF("Failed to initiate iterator for handle_addr_wq\n");
/* Freeing if we are stopping or put back on the addr_wq. */
if (SCTP_BASE_VAR(sctp_pcb_initialized) == 0) {
sctp_asconf_iterator_end(asc, 0);
} else {
LIST_FOREACH(wi, &asc->list_of_work, sctp_nxt_addr) {
LIST_INSERT_HEAD(&SCTP_BASE_INFO(addr_wq), wi, sctp_nxt_addr);
}
SCTP_FREE(asc, SCTP_M_ASC_IT);
}
}
}
}

/*
* start a new iterator
* iterates through all endpoints and associations based on the pcb_state
* flags and asoc_state. "af" (mandatory) is executed for all matching
* assocs and "ef" (optional) is executed when the iterator completes.
* "inpf" (optional) is executed for each new endpoint as it is being
* iterated through. inpe (optional) is called when the inp completes
* its way through all the stcbs.
*/
int
sctp_initiate_iterator(inp_func inpf,
asoc_func af,
inp_func inpe,
uint32_t pcb_state,
uint32_t pcb_features,
uint32_t asoc_state,
void *argp,
uint32_t argi,
end_func ef,
struct sctp_inpcb *s_inp,
uint8_t chunk_output_off)
{
struct sctp_iterator *it = NULL;
if (af == NULL) {
return (-1);
}
if (SCTP_BASE_VAR(sctp_pcb_initialized) == 0) {
SCTP_PRINTF("%s: abort on initialize being %d\n", __func__,
SCTP_BASE_VAR(sctp_pcb_initialized));
return (-1);
}
SCTP_MALLOC(it, struct sctp_iterator *, sizeof(struct sctp_iterator),
SCTP_M_ITER);
if (it == NULL) {
SCTP_LTRACE_ERR_RET(NULL, NULL, NULL, SCTP_FROM_SCTP_PCB, ENOMEM);
return (-1);
}
memset(it, 0, sizeof(*it));
it->function_assoc = af;
it->function_inp = inpf;
if (inpf)
it->done_current_ep = 0;
else
it->done_current_ep = 1;
it->function_atend = ef;
it->pointer = argp;
it->val = argi;
it->pcb_flags = pcb_state;
it->pcb_features = pcb_features;
it->asoc_state = asoc_state;
it->function_inp_end = inpe;
it->no_chunk_output = chunk_output_off;
#if defined(__FreeBSD__) && __FreeBSD_version >= 801000
it->vn = curvnet;
#endif
if (s_inp) {
/* Assume lock is held here */
it->inp = s_inp;
SCTP_INP_INCR_REF(it->inp);
it->iterator_flags = SCTP_ITERATOR_DO_SINGLE_INP;
} else {
SCTP_INP_INFO_RLOCK();
it->inp = LIST_FIRST(&SCTP_BASE_INFO(listhead));
if (it->inp) {
SCTP_INP_INCR_REF(it->inp);
}
SCTP_INP_INFO_RUNLOCK();
it->iterator_flags = SCTP_ITERATOR_DO_ALL_INP;
}
SCTP_IPI_ITERATOR_WQ_LOCK();
if (SCTP_BASE_VAR(sctp_pcb_initialized) == 0) {
SCTP_IPI_ITERATOR_WQ_UNLOCK();
SCTP_PRINTF("%s: rollback on initialize being %d it=%p\n", __func__,
SCTP_BASE_VAR(sctp_pcb_initialized), it);
SCTP_FREE(it, SCTP_M_ITER);
return (-1);
}
TAILQ_INSERT_TAIL(&sctp_it_ctl.iteratorhead, it, sctp_nxt_itr);
if (sctp_it_ctl.iterator_running == 0) {
sctp_wakeup_iterator();
}
SCTP_IPI_ITERATOR_WQ_UNLOCK();
/* sa_ignore MEMLEAK {memory is put on the tailq for the iterator} */
return (0);
}

@tuexen do you think it is possible to accept this PR to make library behave as promised by name of usrsctp_init_nothreads? Thanks a lot in advance!

@mstyura mstyura changed the title Do not start SCTP iterator when initialized via usrsctp_init_nothreads. Do not start SCTP iterator when library is initialized via usrsctp_init_nothreads. May 22, 2020
@mstyura mstyura changed the title Do not start SCTP iterator when library is initialized via usrsctp_init_nothreads. Do not start SCTP iterator thread when library is initialized via usrsctp_init_nothreads. May 22, 2020
@mstyura
Copy link
Contributor Author

mstyura commented May 25, 2020

Dr. @tuexen, could you please have a look at this PR and share your thought if these changes are something that makes sense to accept?
PS: original PR which added single thread API is #339

@tuexen
Copy link
Member

tuexen commented May 25, 2020

The problem I see with this approach is that the sctp_wakeup_iterator() might take a long time now. The idea of the iterator is to delegate work items which might take a long time. With your proposed patch this is not true anymore.

@mstyura
Copy link
Contributor Author

mstyura commented May 25, 2020

Thanks you very much for your time and viewing this PR Dr. @tuexen!

Yes, that also my concern. But when I proposed this change I thought of following:

  1. Primary use case for single threaded API from my point of view are sockets created with AF_CONN - so basically usrsctp is used to prepare sctp packets from user data to being sent by other transport (e.g. DTLS) and assemble data from sctp packets, which are provided by user application. This is solely CPU-bound work in my understanding.
  2. Taking into account the nature of AF_CONN sockets (as far as I understand them) there is by definition no I/O or blocking operations (other than mutex acquisition, which should not be a problem when there is only one thread) is done inside library. Hence any work which could potentially done by SCTP Iterator is CPU-bound and hence it could be done in the same thread as other operations, when library is initialized in the single threaded API.
  3. Even if there is some potentially long running CPU-bound tasks done by SCTP iterator thread then it would be expected to be done on user thread, when library is inited via usrsctp_init_nothreads.

Maybe my understanding of AF_CONN sockets is incorrect and there are still I/O or blocking operations done by library, if so then yes, there is a reason for a dedicated thread, but it still unclear if it is expected behavior when inited via usrsctp_init_nothreads... Maybe signature of method could be changed to accept 2 bools: 1 for SCTP timer thread and 1 for SCTP iterator thread?

Do you think it is possible to change signature of usrsctp_init_nothreads to not to start SCTP timer and have a "true" single threaded API mode?

Thank you very much!

@tuexen
Copy link
Member

tuexen commented May 25, 2020

Thanks you very much for your time and viewing this PR Dr. @tuexen!

Yes, that also my concern. But when I proposed this change I thought of following:

  1. Primary use case for single threaded API from my point of view are sockets created with AF_CONN - so basically usrsctp is used to prepare sctp packets from user data to being sent by other transport (e.g. DTLS) and assemble data from sctp packets, which are provided by user application. This is solely CPU-bound work in my understanding.

I was also assuming for some time that you would use the userland stack in the context of WebRTC (with AF_CONN sockets). For native SCTP you would use a kernel stack. But people are using, as far as I can tell from bug reports, it also for native SCTP usage.

  1. Taking into account the nature of AF_CONN sockets (as far as I understand them) there is by definition no I/O or blocking operations (other than mutex acquisition, which should not be a problem when there is only one thread) is done inside library. Hence any work which could potentially done by SCTP Iterator is CPU-bound and hence it could be done in the same thread as other operations, when library is initialized in the single threaded API.

The question is: When you are using the library in single threaded mode, are you expecting calls to the library taking an arbitrary long time?

  1. Even if there is some potentially long running CPU-bound tasks done by SCTP iterator thread then it would be expected to be done on user thread, when library is inited via usrsctp_init_nothreads.

Maybe my understanding of AF_CONN sockets is incorrect and there are still I/O or blocking operations done by library, if so then yes, there is a reason for a dedicated thread, but it still unclear if it is expected behavior when inited via usrsctp_init_nothreads... Maybe signature of method could be changed to accept 2 bools: 1 for SCTP timer thread and 1 for SCTP iterator thread?

Again, one has to provide a way how the library is expected to be used.

Do you think it is possible to change signature of usrsctp_init_nothreads to not to start SCTP timer and have a "true" single threaded API mode?

Maybe a possible way is to expose a usrsctp_iterator() function, which you have to call periodically and which can run arbitrarily long. Some stuff (like using a recently added address) might only work after calling the usrsctp_iterator(). Would that work for you?

Thank you very much!

@ibc
Copy link

ibc commented Mar 8, 2021

IMHO, definitely a library that is supposed to run in single thread mode (or that provides an option for it, such as usrsctp_init_nothreads) should not run other threads internally, and whatever such a thread does should be exposed to the application so it can provide similar behavior without requiring such a thread.

@tuexen
Copy link
Member

tuexen commented Mar 8, 2021

Please note the usrsctp_init_nothreads() was a contribution. I assumed it was useful for the contributor.

@ibc
Copy link

ibc commented Mar 8, 2021

Please note the usrsctp_init_nothreads() was a contribution. I assumed it was useful for the contributor.

:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants