Mekhanik evgenii/2262 dbg #2312
Conversation
EvgeniiMekhanik
force-pushed
the
MekhanikEvgenii/2262-dbg
branch
2 times, most recently
from
4fc49a9 to
6979f3f
Compare
`tfw_pool_alloc_pages` and `tfw_pool_free_pages` can be called from task context (for example during vhost allocation/deallocation) and from softirq context (during request/response allocation/deallocation). We should protect `pg_cache/pg_next` per cpu variables using `local_bh_disable/local_bh_enable` in this case not `preempt_disable/preempt_enable`. Closes #2262
EvgeniiMekhanik
force-pushed
the
MekhanikEvgenii/2262-dbg
branch
from
cafcdcf to
8417e60
Compare
|
Perhaps, the real question here is: Is the use of tfw_pool functions correct and justified in the vhost case? I have a feeling that the intention for tfw_pool was different. |
| @@ -74,19 +74,19 @@ tfw_pool_alloc_pages(unsigned int order) | |||
| unsigned long pg_res; | |||
There was a problem hiding this comment.
Please update the copyright year
|
Perhaps, my comment was misunderstood, so here's my attempt to assert the thought it expressed. If that's not the case, then please ignore it. The TFW pool was designed as a pure per-CPU pool. "Since Tempesta handles a message only on one CPU, then the pool should not bother about concurrency." - that's what's said in the comment in the source code. This is a very specific design which conforms to Tempesta's design of uninterruptible message processing in one go. However, in vhost code one of the developers didn't quite realize that and used TFW pool as a regular memory pool for allocating chunks of memory to store configuration values. And yes, that code can be and is called from the user context. I think that's the only place where something like that is done. I believe that it's vhost code that should be fixed. It's not an incorrect synchronization in pool.c, it's an incorrect use of TFW pool. Otherwise TFW pool's description in the source code should be reworked. |
During our discussion we decide to don't remove using pool in |
Thanks for the info, that's an excellent point! Perhaps, the description in pool.c should be reworked to reflect that and suggest a yet another use pattern - as opposed to the use of regular in-kernel memory allocation at configuration processing stage for the configuration data. This gives the data a certain locality, even though I am not sure it's going to be all that useful. |
No description provided.