Replace current c-style sessions to c++ #240

tszumski · 2024-10-23T09:34:18Z

No description provided.

Sakoram · 2024-10-23T11:12:35Z

media-proxy/include/session-base.h

+
+#include "utils.h"
+
+class session


Let's capitalize every class name

Do you mean CamelCase or Camel_Snake ?

I don't want to change snake_case, but personally I'm used to class names starting with capital letters, so for classes it would be Snake_case_started_with_capital_letter (however it is called) ^^

I would suggest JustForClassNames, to be consistent with gRPC/TCP class names

Sakoram · 2024-10-23T11:16:05Z

media-proxy/include/session-base.h

+#include <thread>
+#include <stdexcept>
+
+using namespace std; // for atomic_bool


first you write using namespace std; and after that sometimes in code there is std:: used. Let's unify approach.

I would explicitly use std::, it makes obvious that given function is standard one.

Agree, then this change also apply for atomic_bool

yeah, I hope it is possible to do without breaking C code.

Yes, I can do that

Sakoram · 2024-10-23T11:16:27Z

media-proxy/include/session-base.h

+
+#include <mcm_dp.h>
+
+#include "shm_memif.h" /* share memory */


Suggested change

#include "shm_memif.h" /* share memory */

#include "shm_memif.h" /* shared memory */

Sakoram · 2024-10-23T11:21:24Z

media-proxy/include/session-base.h

+    int shm_deinit();
+
+  protected:
+    atomic_bool shm_ready;


you split public section into fields and methods, but in protected section you have both fields and methods. It is a little inconsistent.

Honestly, in my very limited experience I rarely encountered splitting the sections, but I'm ok with it as far as it is consistent.

Sakoram · 2024-10-23T11:22:56Z

media-proxy/include/session-mtl.h

+
+#include "session-base.h"
+
+#define MTL_ZERO_COPY


This flag is added by build script, please remove it from the code.

Previous approach was wrong. Macro is defined in media_proxy subdirectory.
When you add #include "session-mtl.h" inside Unit Tests macro is alwas OFF, while compilation of "media_proxy" submodule happen with macro ON. This lead to smaller object allocation size compared to what library expected, and overall read/write outside of allocation happen

TODO:
Remove ZERO_COPY macro from media_proxy/CMakeLists.txt

Sakoram · 2024-10-23T11:45:05Z

media-proxy/src/api_server_tcp.cc

@@ -134,63 +136,9 @@ void* msg_loop(void* ptr)
                if (it->id == session_id) {
                    /* return memif parameters. */
                    memif_conn_param param = { };
-                    if (it->type == TX) {


beautiful 🎉 I was devastated every time I looked at that spaghetti. good job

Sakoram · 2024-10-23T11:53:36Z

media-proxy/src/api_server_tcp.cc

@@ -134,63 +136,9 @@ void* msg_loop(void* ptr)
                if (it->id == session_id) {


please change Failed to start MTL session. to Failed to start a session. a few lines above. ^^^^
Thanks. :)

Sakoram · 2024-10-23T12:05:31Z

media-proxy/src/session-base.cc

+    int err;
+    memif_socket_handle_t memif_socket = memif_conn_args.socket;
+
+    do {


I think we should implement graceful exit here. Currently we use pthread_cancel(), which could potentially create some problems as this thread handles events and calls functions like on_receive_cb(). Those functions lock mutexes, and interact with libfabric and MTL. A lot of funny stuff can happen when this is interrupted in random places.

We could set a little shorter timeout then infinity and check some new stop flag on every loop.

Sakoram · 2024-10-23T12:36:32Z

media-proxy/src/session-base.cc

+    int ret = 0;
+
+    /* Set application name */
+    strncpy(memif_socket_args.app_name, memif_ops->app_name,


Suggested change

strncpy(memif_socket_args.app_name, memif_ops->app_name,

strlcpy(memif_socket_args.app_name, memif_ops->app_name,

to be sure that the string is NULL terminated. Should be done everywhere.

Sakoram · 2024-10-23T13:03:06Z

media-proxy/src/session-mtl-st2110-20.cc

+void rx_st20_mtl_session::cleanup()
+{
+    if (frame_thread_handle) {
+        frame_thread_handle->join();


I think it won't cancel the thread. It would wait indefinitely. The stop variable has to be set for this thread to stop.

The same issue exists in different places

stop variable is set in session_stop(), but i think i should completely remove that function and move everything to destructor

but session_stop() is called from proxy_context.cc, and this join() will freeze the thread that called rx_st20_mtl_session(), which is the thread from proxy_context.cc which would call session_stop() otherwise.

Anyway, you are right. We need to leverage RAII as much as possible, we should move to destructors every cleanup that can be moved there.

In this case, to cleanup when constructor fails and throws an exception we could:

Use smart pointers whenever you allocate memory

Do something like this when smart pointers aren't enough:

class ResourceWrapper { // Your resource management logic public: ResourceWrapper () { // Acquire the resource } ~ResourceWrapper () { // Release the resource } }; class Session{ ResourceWrapper resource; // RAII type public: Session() { // If an exception is thrown here, `resource` will still be cleaned up if (/* some condition */) { throw std::runtime_error("Initialization failed"); } } ~Session() { // `resource ` will automatically clean itself up } };

Sakoram · 2024-10-23T13:24:05Z

I'm thinking what to do with error handling.

In some places there is simply return -1 used. I don't like it.
Elsewhere, some semi-useful error codes ( like return -EINVAL ) are returned. That's better, the best what can be done in C, maybe only defining more descriptive err codes could improve the situation in C
But this is c++ and sometimes throw is used, which seems to be the most elegant way. maybe slightly less performant, but not much, I guess it is the best everywhere besides critical paths.

Any thoughts? Are there any drawback of using try-catch and throw everywhere?

Sakoram · 2024-10-23T13:39:29Z

media-proxy/src/session-mtl-st2110-20.cc

+        return -1;
+    }
+
+    err = memif_buffer_alloc_timeout(memif_conn, qid, shm_bufs, 1, &rx_buf_num, frame_size, 10);


I realized a problem that I myself had introduced. This code is invoked from MTL's "scheduler" thread. This function has to be very quick and non blocking. I'm not even sure if normal memif_buffer_alloc can be used in it, but I'm sure that using memif_buffer_alloc_timeout is a very bad idea as this function could wait for slot in memif's ring and this will starve every session in MTL's scheduler that made call to this function.

I guess we have to use memif_buffer_alloc() instead of memif_buffer_alloc_timeout() and just drop frames if no memory is available in memif ring

Sakoram · 2024-10-23T13:44:09Z

What do you think about splitting every class to a separate file? It could improve readability

Sakoram · 2024-10-23T13:55:06Z

media-proxy/src/proxy_context.cc

+    session_ptr->type = RX;
+    mDpCtx.push_back(session_ptr);
+
+    return session_ptr->id;
 }

 int ProxyContext::RxStart_mtl(const mcm_conn_param *request)


Break this function into a few smaller ones. for example move device init into first function, move the case switch to second, call the two of them and set variables in this one.

change imtl_init_preparing to some real lock

the same applies to TxStart_mtl

please add a lock similar to imtl_init_preparing (but actually working) also to RxStart_rdma and TxStart_rdma

The "case switch part" could be even a separate class as it is basically a factory pattern. Calling it explicitly a factory could help readability.
(if we want to be very crazy, we could even use abstract factory with concrete factories for RX and TX. I guess this could also work, but this is a lot of abstraction)

Any bigger changes to proxy_context.cc and api_server_tcp.cc should be done in the next PR to not to touch too many topics at once

Break this function into a few smaller ones. for example move device init into first function, move the case switch to second, call the two of them and set variables in this one.

change imtl_init_preparing to some real lock

the same applies to TxStart_mtl

please add a lock similar to imtl_init_preparing (but actually working) also to RxStart_rdma and TxStart_rdma

I think the best way is to move MTL/RDMA device creation before we accept any connection(both of them at once), and close those devices after we stop accepting connection. This way we avoid unnecessary locks and imtl_init_preparing will disappear. Thanks to that only mDpCtx have to be locked

I think that we can define some arguments to media_proxy to specify what backend will be initialized at startup. maybe everything currently available should be initialized by default.

Sakoram · 2024-10-23T14:20:14Z

media-proxy/src/proxy_context.cc


+        session *session_ptr = *ctx;
+        session_ptr->session_stop();
+        delete session_ptr;
        mDpCtx.erase(ctx);


I know that you know that, but just a reminder to lock mDpCtx every time it is changed.

Any bigger changes to proxy_context.cc and api_server_tcp.cc should be done in the next PR to not to touch too many topics at once

tszumski · 2024-10-24T05:30:32Z

I'm thinking what to do with error handling.

In some places there is simply return -1 used. I don't like it.

Elsewhere, some semi-useful error codes ( like return -EINVAL ) are returned. That's better, the best what can be done in C, maybe only defining more descriptive err codes could improve the situation in C

But this is c++ and sometimes throw is used, which seems to be the most elegant way. maybe slightly less performant, but not much, I guess it is the best everywhere besides critical paths.

Any thoughts? Are there any drawback of using try-catch and throw everywhere?

We cannot use "throw" functionality in function that are called through callback by MTL/memif. We can only return error codes or print error messages
Generally for proper error(and messages/logs) handling we have to implement one consistent approach across entire MCM repository. I think this is quite large topic and should added as another PR

tszumski · 2024-10-24T05:38:03Z

What do you think about splitting every class to a separate file? It could improve readability

Good idea, only if we move source files into multiple subdirectories. Otherwise having too many files in single directory will reduce readability
My proposition:

media_proxy
    app
        media_proxy.cc
    servers
        tcp
            <tcp-related sources>
        grpc
            <grpc-related sources>
    sessions
        rdma
            <rdma-related sources>
        mtl
            <mtl-related sources>

tszumski added 3 commits October 23, 2024 09:30

Add mtl st20/22/30 rx/tx session

50bfe3e

Add rdma rx/tx session

f824619

Enable new session in media_proxy code

66db432

tszumski requested review from pwysock1-intel and soopel as code owners October 23, 2024 09:34

Remove unused code

13b0b5d

Sakoram reviewed Oct 23, 2024

View reviewed changes


		#include <mcm_dp.h>

		#include "shm_memif.h" /* share memory */

	#include "shm_memif.h" /* share memory */
	#include "shm_memif.h" /* shared memory */

		@@ -134,63 +136,9 @@ void* msg_loop(void* ptr)
		if (it->id == session_id) {

	strncpy(memif_socket_args.app_name, memif_ops->app_name,
	strlcpy(memif_socket_args.app_name, memif_ops->app_name,

Replace current c-style sessions to c++ #240

Are you sure you want to change the base?

Replace current c-style sessions to c++ #240

Conversation

tszumski commented Oct 23, 2024

Sakoram Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tszumski Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tszumski Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sakoram Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tszumski Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sakoram Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Sakoram commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sakoram commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tszumski Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Sakoram Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tszumski commented Oct 24, 2024

tszumski commented Oct 24, 2024

Sakoram Oct 23, 2024 •

edited

Loading

tszumski Oct 23, 2024 •

edited

Loading

tszumski Oct 23, 2024 •

edited

Loading

Sakoram Oct 23, 2024 •

edited

Loading

tszumski Oct 24, 2024 •

edited

Loading

Sakoram Oct 24, 2024 •

edited

Loading

tszumski Oct 24, 2024 •

edited

Loading

Sakoram Oct 24, 2024 •

edited

Loading