pulkin / micropython

MicroPython implementation on Ai-Thinker GPRS module A9 (RDA8955)
https://micropython.org
MIT License
103 stars 30 forks source link

Memory leak and potential race. #69

Open bokolob opened 3 years ago

bokolob commented 3 years ago

1) Here - https://github.com/pulkin/micropython/blob/b84c71917188f6c2fdc6f1c31d526fd150fc2f57/ports/gprs_a9/modcellular.c#L533

We don't free memory, just allocate a new list. I suppose it can be a memory leak. I'm not sure about freeing memory in MP (didn't find it either in documentation and code yet).

I'd like to rewrite this part as list_clear(sms_list_buffer).

2) modcellular_notify_sms_list is called from another thread. As I understood we don't have any documentation describing tasks on a9g platform. But I believe that they are threads :) So, we need to use a mutex or semaphore here, because sms_list_buffer might be modified from different threads (event loop thread and MP program thread).

3) Probably all variables that can be accessed from event loop thread should be declared as volatile.

bokolob commented 3 years ago

I think we have to define

define MICROPY_BEGIN_ATOMIC_SECTION() and #define MICROPY_END_ATOMIC_SECTION(state)

bokolob commented 3 years ago

I'd like to change sms_list API - make it completely async (run callback on each _LIST event). I suppose it would be more reliable, than having a list which is filled from another thread. Like - cellular.SMS.list(callback) But as I understood mp_schedule_task has some limitations in queue size.

pulkin commented 3 years ago

You should look for garbage collector in mpy to sort out possible memory leak issues. I seem to have enabled it and it seems to work so everything inside mpy heap including objects mp_obj_new* will be taken care of.

I have only vague understanding about threads on a9g. The only observation I can share here is that context changes are done when OS_Sleep is explicitly called with an argument above some threshold. I do not remember the details, though.

bokolob commented 3 years ago

I have only vague understanding about threads on a9g. The only observation I can share here is that context changes are done when OS_Sleep is explicitly called with an argument above some threshold. I do not remember the details, though.

It's definitely not the only reason for context switch :)

About GC - if the list is just reallocated (pointer is just overwritten), how GC can get noticed that its memory is not used anymore? I think it's impossible for GC at least in C. AFAIK python uses reference counting, and before rewriting pointer it's needed to decrement counter. Then it will know that object isn't in use anymore.

bokolob commented 3 years ago

And, if GC is switched off, has free to be called explicitly?

It seems that m_del() /m_renew() should be called to release/reallocate memory.

pulkin commented 3 years ago

They might give a more educated answer on micropython forum. There is also a brief summary of memory manager.

What I learned is that mpy heap is a contiguous range of addresses split into blocks monitored by gc. You also define the stack memory range when initializing the gc. When sweeping the whole stack (and CPU regs) is/are searched for valid 32-bit addresses pointing into the heap. Every memory block having no reference on stack is freed.

sms_list_buffer is not a stack variable so there might be a problem there. Otherwise you may easily check that gc actually works by creating a large memory chunk and freeing it via gc.collect.

If gc is switched off you will eventually run out of memory.

bokolob commented 3 years ago

Otherwise you may easily check that gc actually works by creating a large memory chunk and freeing it via gc.collect

I'm sure that it works, but I think that here is no magic, and as we use C, not any of managed languages, GC needs our help to recognize when memory is not used