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.

Sign up for GitHub

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

Jump to bottom

SmartPtr: Support detect memory leak by valgrind. v6.0.132 #4102

Merged
merged 6 commits into from
Jun 29, 2024
Merged

SmartPtr: Support detect memory leak by valgrind. v6.0.132 #4102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
30eded4
SmartPtr: Support detect memory leak by valgrind.
winlinvip Jun 21, 2024
adcfb37
Fix build fail for thread local debug stat.
winlinvip Jun 21, 2024
69f1998
SmartPtr: Free the http handler entry.
winlinvip Jun 24, 2024
ca3eaf1
SmartPtr: Free the stack of ST.
winlinvip Jun 24, 2024
ff9b0fb
Fix bugs.
winlinvip Jun 29, 2024
3efecaa
Update release to v6.0.132
winlinvip Jun 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions trunk/3rdparty/st-srs/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,9 @@ endif
#
# make EXTRA_CFLAGS=-DDEBUG_STATS
#
# or cache the stack and reuse it:
# make EXTRA_CFLAGS=-DMD_CACHE_STACK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not add configure args to bypass MD_CACHE_STACK to st-srs, like MD_VALGRIND?

Copy link
Member Author

@winlinvip winlinvip Jun 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to not cache the stack by default, while this macro is used to enable the stack cache.

Won't fix.

#
# or enable the coverage for utest:
# make UTEST_FLAGS="-fprofile-arcs -ftest-coverage"
#
Expand Down
31 changes: 25 additions & 6 deletions trunk/3rdparty/st-srs/stk.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,15 @@ __thread int _st_num_free_stacks = 0;
__thread int _st_randomize_stacks = 0;

static char *_st_new_stk_segment(int size);
static void _st_delete_stk_segment(char *vaddr, int size);

_st_stack_t *_st_stack_new(int stack_size)
{
_st_clist_t *qp;
_st_stack_t *ts;
int extra;


#ifdef MD_CACHE_STACK
for (qp = _st_free_stacks.next; qp != &_st_free_stacks; qp = qp->next) {
ts = _ST_THREAD_STACK_PTR(qp);
if (ts->stk_size >= stack_size) {
Expand All @@ -75,11 +77,31 @@ _st_stack_t *_st_stack_new(int stack_size)
return ts;
}
}
#endif

extra = _st_randomize_stacks ? _ST_PAGE_SIZE : 0;
#ifndef MD_CACHE_STACK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if don't cache stack, why not free _st_stack_t in _st_stack_free, don't use _st_free_stacks to store the freed stack?
The stack still not freed in _st_free_stacks, else a new coroutine started and request a new stack here.

Copy link
Member Author

@winlinvip winlinvip Jun 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The crash appears to occur when the st is freed within the _st_stack_free function, which seems to be happening while the stack is still in use. Although it is added to the _st_free_stacks structure, and the previous st is released before the next coroutine starts, this approach should not result in a significant memory leak issue.

Won't fix.

TRANS_BY_GPT4

for (qp = _st_free_stacks.next; qp != &_st_free_stacks;) {
ts = _ST_THREAD_STACK_PTR(qp);
// Before qp is freed, move to next one, because the qp will be freed when free the ts.
qp = qp->next;

ST_REMOVE_LINK(&ts->links);
_st_num_free_stacks--;

#if defined(DEBUG) && !defined(MD_NO_PROTECT)
mprotect(ts->vaddr, REDZONE, PROT_READ | PROT_WRITE);
mprotect(ts->stk_top + extra, REDZONE, PROT_READ | PROT_WRITE);
#endif

_st_delete_stk_segment(ts->vaddr, ts->vaddr_size);
free(ts);
}
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only rarely a few part of codes in st-srs, add comments to denote which #if to match.

Suggested change
#endif
#endif /* MD_CACHE_STACK */

I would suggest add comments in this way.


/* Make a new thread stack object. */
if ((ts = (_st_stack_t *)calloc(1, sizeof(_st_stack_t))) == NULL)
return NULL;
extra = _st_randomize_stacks ? _ST_PAGE_SIZE : 0;
ts->vaddr_size = stack_size + 2*REDZONE + extra;
ts->vaddr = _st_new_stk_segment(ts->vaddr_size);
if (!ts->vaddr) {
Expand Down Expand Up @@ -114,7 +136,7 @@ void _st_stack_free(_st_stack_t *ts)
{
if (!ts)
return;

/* Put the stack on the free list */
ST_APPEND_LINK(&ts->links, _st_free_stacks.prev);
_st_num_free_stacks++;
Expand Down Expand Up @@ -152,8 +174,6 @@ static char *_st_new_stk_segment(int size)
}


/* Not used */
#if 0
void _st_delete_stk_segment(char *vaddr, int size)
winlinvip marked this conversation as resolved.
Show resolved Hide resolved
{
#ifdef MALLOC_STACK
Expand All @@ -162,7 +182,6 @@ void _st_delete_stk_segment(char *vaddr, int size)
(void) munmap(vaddr, size);
#endif
}
#endif

int st_randomize_stacks(int on)
{
Expand Down
1 change: 1 addition & 0 deletions trunk/doc/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ The changelog for SRS.
<a name="v6-changes"></a>

## SRS 6.0 Changelog
* v6.0, 2024-06-29, Merge [#4102](https://github.com/ossrs/srs/pull/4102): SmartPtr: Support detect memory leak by valgrind. v6.0.132 (#4102)
* v6.0, 2024-06-21, Merge [#4100](https://github.com/ossrs/srs/pull/4100): Fix security scan problems. v6.0.131 (#4100)
* v6.0, 2024-06-21, Merge [#4097](https://github.com/ossrs/srs/pull/4097): SmartPtr: Support load test for source by srs-bench. v6.0.130 (#4097)
* v6.0, 2024-06-15, Merge [#4089](https://github.com/ossrs/srs/pull/4089): SmartPtr: Support shared ptr for live source. v6.0.129 (#4089)
Expand Down
1 change: 1 addition & 0 deletions trunk/research/st/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ udp-server
udp-client
cost
cost.log
thread-join
36 changes: 36 additions & 0 deletions trunk/research/st/thread-join.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
g++ thread-join.cpp ../../objs/st/libst.a -g -O0 -o thread-join && ./thread-join
*/
#include <stdio.h>
#include <stdlib.h>
#include "../../objs/st/st.h"

void* pfn(void* arg) {
printf("pid=%d, coroutine is ok\n", ::getpid());
return NULL;
}

int main(int argc, char** argv) {
st_init();

printf("pid=%d, create coroutine #1\n", ::getpid());
st_thread_t thread = st_thread_create(pfn, NULL, 1, 0);
st_thread_join(thread, NULL);

st_usleep(100 * 1000);

printf("pid=%d, create coroutine #2\n", ::getpid());
thread = st_thread_create(pfn, NULL, 1, 0);
st_thread_join(thread, NULL);

st_usleep(100 * 1000);

printf("pid=%d, create coroutine #3\n", ::getpid());
thread = st_thread_create(pfn, NULL, 1, 0);
st_thread_join(thread, NULL);

printf("done\n");
st_thread_exit(NULL);
return 0;
}

2 changes: 1 addition & 1 deletion trunk/src/app/srs_app_conn.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ SrsResourceManager::~SrsResourceManager()
trd->stop();

srs_freep(trd);
srs_cond_destroy(cond);
}
srs_cond_destroy(cond);

clear();

Expand Down
2 changes: 1 addition & 1 deletion trunk/src/app/srs_app_hourglass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ void SrsFastTimer::unsubscribe(ISrsFastTimer* timer)
{
vector<ISrsFastTimer*>::iterator it = std::find(handlers_.begin(), handlers_.end(), timer);
if (it != handlers_.end()) {
it = handlers_.erase(it);
handlers_.erase(it);
}
}

Expand Down
7 changes: 4 additions & 3 deletions trunk/src/app/srs_app_http_stream.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1073,9 +1073,6 @@ void SrsHttpStreamServer::http_unmount(SrsRequest* r)
SrsBufferCache* cache = entry->cache;
SrsAutoFree(SrsBufferCache, cache);

// Unmount the HTTP handler.
mux.unhandle(entry->mount, stream);

// Notify cache and stream to stop.
if (stream->entry) stream->entry->enabled = false;
cache->stop();
Expand All @@ -1089,6 +1086,10 @@ void SrsHttpStreamServer::http_unmount(SrsRequest* r)
srs_usleep(100 * SRS_UTIME_MILLISECONDS);
}

// Unmount the HTTP handler, which will free the entry. Note that we must free it after cache and
// stream stopped for it uses it.
mux.unhandle(entry->mount, stream);

srs_trace("http: unmount flv stream for sid=%s, i=%d", sid.c_str(), i);
}

Expand Down
86 changes: 43 additions & 43 deletions trunk/src/app/srs_app_hybrid.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,55 +25,55 @@ extern SrsPps* _srs_pps_conn;
extern SrsPps* _srs_pps_dispose;

#if defined(SRS_DEBUG) && defined(SRS_DEBUG_STATS)
extern unsigned long long _st_stat_recvfrom;
extern unsigned long long _st_stat_recvfrom_eagain;
extern unsigned long long _st_stat_sendto;
extern unsigned long long _st_stat_sendto_eagain;
extern __thread unsigned long long _st_stat_recvfrom;
extern __thread unsigned long long _st_stat_recvfrom_eagain;
extern __thread unsigned long long _st_stat_sendto;
extern __thread unsigned long long _st_stat_sendto_eagain;
SrsPps* _srs_pps_recvfrom = NULL;
SrsPps* _srs_pps_recvfrom_eagain = NULL;
SrsPps* _srs_pps_sendto = NULL;
SrsPps* _srs_pps_sendto_eagain = NULL;

extern unsigned long long _st_stat_read;
extern unsigned long long _st_stat_read_eagain;
extern unsigned long long _st_stat_readv;
extern unsigned long long _st_stat_readv_eagain;
extern unsigned long long _st_stat_writev;
extern unsigned long long _st_stat_writev_eagain;
extern __thread unsigned long long _st_stat_read;
extern __thread unsigned long long _st_stat_read_eagain;
extern __thread unsigned long long _st_stat_readv;
extern __thread unsigned long long _st_stat_readv_eagain;
extern __thread unsigned long long _st_stat_writev;
extern __thread unsigned long long _st_stat_writev_eagain;
SrsPps* _srs_pps_read = NULL;
SrsPps* _srs_pps_read_eagain = NULL;
SrsPps* _srs_pps_readv = NULL;
SrsPps* _srs_pps_readv_eagain = NULL;
SrsPps* _srs_pps_writev = NULL;
SrsPps* _srs_pps_writev_eagain = NULL;

extern unsigned long long _st_stat_recvmsg;
extern unsigned long long _st_stat_recvmsg_eagain;
extern unsigned long long _st_stat_sendmsg;
extern unsigned long long _st_stat_sendmsg_eagain;
extern __thread unsigned long long _st_stat_recvmsg;
extern __thread unsigned long long _st_stat_recvmsg_eagain;
extern __thread unsigned long long _st_stat_sendmsg;
extern __thread unsigned long long _st_stat_sendmsg_eagain;
SrsPps* _srs_pps_recvmsg = NULL;
SrsPps* _srs_pps_recvmsg_eagain = NULL;
SrsPps* _srs_pps_sendmsg = NULL;
SrsPps* _srs_pps_sendmsg_eagain = NULL;

extern unsigned long long _st_stat_epoll;
extern unsigned long long _st_stat_epoll_zero;
extern unsigned long long _st_stat_epoll_shake;
extern unsigned long long _st_stat_epoll_spin;
extern __thread unsigned long long _st_stat_epoll;
extern __thread unsigned long long _st_stat_epoll_zero;
extern __thread unsigned long long _st_stat_epoll_shake;
extern __thread unsigned long long _st_stat_epoll_spin;
SrsPps* _srs_pps_epoll = NULL;
SrsPps* _srs_pps_epoll_zero = NULL;
SrsPps* _srs_pps_epoll_shake = NULL;
SrsPps* _srs_pps_epoll_spin = NULL;

extern unsigned long long _st_stat_sched_15ms;
extern unsigned long long _st_stat_sched_20ms;
extern unsigned long long _st_stat_sched_25ms;
extern unsigned long long _st_stat_sched_30ms;
extern unsigned long long _st_stat_sched_35ms;
extern unsigned long long _st_stat_sched_40ms;
extern unsigned long long _st_stat_sched_80ms;
extern unsigned long long _st_stat_sched_160ms;
extern unsigned long long _st_stat_sched_s;
extern __thread unsigned long long _st_stat_sched_15ms;
extern __thread unsigned long long _st_stat_sched_20ms;
extern __thread unsigned long long _st_stat_sched_25ms;
extern __thread unsigned long long _st_stat_sched_30ms;
extern __thread unsigned long long _st_stat_sched_35ms;
extern __thread unsigned long long _st_stat_sched_40ms;
extern __thread unsigned long long _st_stat_sched_80ms;
extern __thread unsigned long long _st_stat_sched_160ms;
extern __thread unsigned long long _st_stat_sched_s;
SrsPps* _srs_pps_sched_15ms = NULL;
SrsPps* _srs_pps_sched_20ms = NULL;
SrsPps* _srs_pps_sched_25ms = NULL;
Expand All @@ -96,11 +96,12 @@ SrsPps* _srs_pps_clock_160ms = NULL;
SrsPps* _srs_pps_timer_s = NULL;

#if defined(SRS_DEBUG) && defined(SRS_DEBUG_STATS)
extern int _st_active_count;
extern unsigned long long _st_stat_thread_run;
extern unsigned long long _st_stat_thread_idle;
extern unsigned long long _st_stat_thread_yield;
extern unsigned long long _st_stat_thread_yield2;
extern __thread int _st_active_count;
extern __thread int _st_num_free_stacks;
extern __thread unsigned long long _st_stat_thread_run;
extern __thread unsigned long long _st_stat_thread_idle;
extern __thread unsigned long long _st_stat_thread_yield;
extern __thread unsigned long long _st_stat_thread_yield2;
SrsPps* _srs_pps_thread_run = NULL;
SrsPps* _srs_pps_thread_idle = NULL;
SrsPps* _srs_pps_thread_yield = NULL;
Expand Down Expand Up @@ -135,19 +136,20 @@ SrsHybridServer::SrsHybridServer()

SrsHybridServer::~SrsHybridServer()
{
srs_freep(clock_monitor_);

srs_freep(timer20ms_);
srs_freep(timer100ms_);
srs_freep(timer1s_);
srs_freep(timer5s_);

// We must free servers first, because it may depend on the timers of hybrid server.
vector<ISrsHybridServer*>::iterator it;
for (it = servers.begin(); it != servers.end(); ++it) {
ISrsHybridServer* server = *it;
srs_freep(server);
}
servers.clear();

srs_freep(clock_monitor_);

srs_freep(timer20ms_);
srs_freep(timer100ms_);
srs_freep(timer1s_);
srs_freep(timer5s_);
}

void SrsHybridServer::register_server(ISrsHybridServer* svr)
Expand Down Expand Up @@ -237,8 +239,6 @@ void SrsHybridServer::stop()
ISrsHybridServer* server = *it;
server->stop();
}

srs_st_destroy();
}

SrsServerAdapter* SrsHybridServer::srs()
Expand Down Expand Up @@ -372,8 +372,8 @@ srs_error_t SrsHybridServer::on_timer(srs_utime_t interval)
#if defined(SRS_DEBUG) && defined(SRS_DEBUG_STATS)
_srs_pps_thread_run->update(_st_stat_thread_run); _srs_pps_thread_idle->update(_st_stat_thread_idle);
_srs_pps_thread_yield->update(_st_stat_thread_yield); _srs_pps_thread_yield2->update(_st_stat_thread_yield2);
if (_st_active_count > 0 || _srs_pps_thread_run->r10s() || _srs_pps_thread_idle->r10s() || _srs_pps_thread_yield->r10s() || _srs_pps_thread_yield2->r10s()) {
snprintf(buf, sizeof(buf), ", co=%d,%d,%d, yield=%d,%d", _st_active_count, _srs_pps_thread_run->r10s(), _srs_pps_thread_idle->r10s(), _srs_pps_thread_yield->r10s(), _srs_pps_thread_yield2->r10s());
if (_st_active_count > 0 || _st_num_free_stacks > 0 || _srs_pps_thread_run->r10s() || _srs_pps_thread_idle->r10s() || _srs_pps_thread_yield->r10s() || _srs_pps_thread_yield2->r10s()) {
snprintf(buf, sizeof(buf), ", co=%d,%d,%d, stk=%d, yield=%d,%d", _st_active_count, _srs_pps_thread_run->r10s(), _srs_pps_thread_idle->r10s(), _st_num_free_stacks, _srs_pps_thread_yield->r10s(), _srs_pps_thread_yield2->r10s());
thread_desc = buf;
}
#endif
Expand Down
9 changes: 5 additions & 4 deletions trunk/src/app/srs_app_ingest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ SrsIngester::SrsIngester()
disposed = false;

trd = new SrsDummyCoroutine();
pprint = SrsPithyPrint::create_ingester();
pprint_ = SrsPithyPrint::create_ingester();
}

SrsIngester::~SrsIngester()
Expand All @@ -103,6 +103,7 @@ SrsIngester::~SrsIngester()

srs_freep(trd);
clear_engines();
srs_freep(pprint_);
}

void SrsIngester::dispose()
Expand Down Expand Up @@ -466,7 +467,7 @@ srs_error_t SrsIngester::initialize_ffmpeg(SrsFFMPEG* ffmpeg, SrsConfDirective*

void SrsIngester::show_ingest_log_message()
{
pprint->elapse();
pprint_->elapse();

if ((int)ingesters.size() <= 0) {
return;
Expand All @@ -477,9 +478,9 @@ void SrsIngester::show_ingest_log_message()
SrsIngesterFFMPEG* ingester = ingesters.at(index);

// reportable
if (pprint->can_print()) {
if (pprint_->can_print()) {
srs_trace("-> " SRS_CONSTS_LOG_INGESTER " time=%dms, ingesters=%d, #%d(alive=%dms, %s)",
srsu2msi(pprint->age()), (int)ingesters.size(), index, srsu2msi(ingester->alive()), ingester->uri().c_str());
srsu2msi(pprint_->age()), (int)ingesters.size(), index, srsu2msi(ingester->alive()), ingester->uri().c_str());
}
}

Expand Down
2 changes: 1 addition & 1 deletion trunk/src/app/srs_app_ingest.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class SrsIngester : public ISrsCoroutineHandler, public ISrsReloadHandler
std::vector<SrsIngesterFFMPEG*> ingesters;
private:
SrsCoroutine* trd;
SrsPithyPrint* pprint;
SrsPithyPrint* pprint_;
// Whether the ingesters are expired, for example, the listen port changed,
// all ingesters must be restart.
bool expired;
Expand Down
Loading
Loading