Skip to content

Conversation

@zhhyu7
Copy link
Contributor

@zhhyu7 zhhyu7 commented Dec 30, 2025

Summary

remove the use of net_lock in the local module and decouple it from other network modules.

Impact

net::local

Testing

sim:matter with test code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <errno.h>
#include <sys/socket.h>
#include <sys/un.h>
#include <sys/wait.h>

#define SOCKET_PATH "local_socket"
#define BUFFER_SIZE 1024

// Cleanup function: delete socket file
static void cleanup_socket(const char *path) {
    if (unlink(path) == -1 && errno != ENOENT) {
        perror("Failed to delete socket file");
    }
}

// Server function
static int run_server(void) {
    int server_fd, client_fd;
    struct sockaddr_un server_addr, client_addr;
    socklen_t client_len;
    char buffer[BUFFER_SIZE];
    ssize_t bytes;
    
    printf("[Server] Creating PF_UNIX SOCK_STREAM socket...\n");
    server_fd = socket(PF_UNIX, SOCK_STREAM, 0);
    if (server_fd < 0) {
        perror("Failed to create server socket");
        return -1;
    }
    
    // Clean up old socket file if it exists
    cleanup_socket(SOCKET_PATH);
    
    // Set up server address
    memset(&server_addr, 0, sizeof(server_addr));
    server_addr.sun_family = AF_UNIX;
    strncpy(server_addr.sun_path, SOCKET_PATH, sizeof(server_addr.sun_path) - 1);
    
    printf("[Server] Binding to path: %s\n", SOCKET_PATH);
    if (bind(server_fd, (struct sockaddr *)&server_addr, sizeof(server_addr)) < 0) {
        perror("Bind failed");
        close(server_fd);
        return -1;
    }
    
    printf("[Server] Starting to listen for connections...\n");
    if (listen(server_fd, 5) < 0) {
        perror("Listen failed");
        close(server_fd);
        return -1;
    }
    
    // Accept client connection
    client_len = sizeof(client_addr);
    printf("[Server] Waiting for client connection...\n");
    client_fd = accept(server_fd, (struct sockaddr *)&client_addr, &client_len);
    if (client_fd < 0) {
        perror("Accept failed");
        close(server_fd);
        return -1;
    }
    printf("[Server] Client connected\n");
    
    // Receive message from client
    memset(buffer, 0, BUFFER_SIZE);
    bytes = recv(client_fd, buffer, BUFFER_SIZE - 1, 0);
    if (bytes < 0) {
        perror("Failed to receive message");
        close(client_fd);
        close(server_fd);
        return -1;
    }
    printf("[Server] Received message from client: %s\n", buffer);
    
    // Send response to client
    const char *response = "Hello client, I am the server!";
    printf("[Server] Sending response: %s\n", response);
    if (send(client_fd, response, strlen(response), 0) < 0) {
        perror("Failed to send response");
        close(client_fd);
        close(server_fd);
        return -1;
    }
    
    // Wait for client to finish processing
    sleep(1);
    
    // Cleanup
    close(client_fd);
    close(server_fd);
    
    return 0;
}

// Client function
static int run_client(void) {
    int client_fd;
    struct sockaddr_un server_addr;
    char buffer[BUFFER_SIZE];
    ssize_t bytes;
    
    // Wait for server to be ready
    sleep(1);
    
    printf("[Client] Creating PF_UNIX SOCK_STREAM socket...\n");
    client_fd = socket(PF_UNIX, SOCK_STREAM, 0);
    if (client_fd < 0) {
        perror("Failed to create client socket");
        return -1;
    }
    
    // Set up server address
    memset(&server_addr, 0, sizeof(server_addr));
    server_addr.sun_family = AF_UNIX;
    strncpy(server_addr.sun_path, SOCKET_PATH, sizeof(server_addr.sun_path) - 1);
    
    printf("[Client] Connecting to server path: %s\n", SOCKET_PATH);
    if (connect(client_fd, (struct sockaddr *)&server_addr, sizeof(server_addr)) < 0) {
        perror("Failed to connect to server");
        close(client_fd);
        return -1;
    }
    printf("[Client] Connected to server\n");
    
    // Send message to server
    const char *message = "Hello server, I am the client!";
    printf("[Client] Sending message: %s\n", message);
    if (send(client_fd, message, strlen(message), 0) < 0) {
        perror("Failed to send message");
        close(client_fd);
        return -1;
    }
    
    // Receive response from server
    memset(buffer, 0, BUFFER_SIZE);
    bytes = recv(client_fd, buffer, BUFFER_SIZE - 1, 0);
    if (bytes < 0) {
        perror("Failed to receive response");
        close(client_fd);
        return -1;
    }
    printf("[Client] Received response from server: %s\n", buffer);
    
    // Cleanup
    close(client_fd);
    
    return 0;
}

int main(int argc, char *argv[]) {
    pid_t pid;
    
    printf("=== PF_UNIX SOCK_STREAM Socket Communication Example ===\n");
    
    // Create child process
    pid = fork();
    if (pid < 0) {
        perror("Failed to create child process");
        return EXIT_FAILURE;
    }
    
    if (pid == 0) {
        // Child process: client
        printf("\n--- Client Process (PID: %d) ---\n", getpid());
        if (run_client() < 0) {
            printf("Client execution failed\n");
        }
        printf("Client process ended\n");
        exit(0);
    } else {
        // Parent process: server
        printf("\n--- Server Process (PID: %d) ---\n", getpid());
        if (run_server() < 0) {
            printf("Server execution failed\n");
        }
        
        // Wait for child process to finish
        waitpid(pid, NULL, 0);
        
        // Cleanup socket file
        cleanup_socket(SOCKET_PATH);
        
        printf("\nServer process ended\n");
        printf("=== Program execution completed ===\n");
    }
    
    return EXIT_SUCCESS;
}

NuttX log:

NuttShell (NSH) NuttX-12.12.0-RC0
MOTD: username=admin password=Administrator
nsh> hello
=== PF_UNIX SOCK_STREAM Socket Communication Example ===

--- Server Process (PID: 4) ---
[Server] Creating PF_UNIX SOCK_STREAM socket...
[Server] Binding to path: local_socket
[Server] Starting to listen for connections...
[Server] Waiting for client connection...

--- Client Process (PID: 5) ---
[Client] Creating PF_UNIX SOCK_STREAM socket...
[Client] Connecting to server path: local_socket
[Client] Connected to server
[Client] Sending message: Hello server, I am the client!
[Server] Client connected
[Server] Received message from client: Hello server, I am the client!
[Server] Sending response: Hello client, I am the server!
[Client] Received response from server: Hello client, I am the server!
Client process ended

Server process ended
=== Program execution completed ===
nsh> 
dup2 and setsockopt can use the lock in conn to protect resources,
the lock in accept is originally used to protect the connection status.
however, only the send, recv, netpoll, and connect processes will
check this flag. only when the interface returns will the corresponding
conn structure be exposed to the caller, and then the above operations
can be performed. Therefore, this net_lock is not necessary.

Signed-off-by: zhanghongyu <zhanghongyu@xiaomi.com>
remove the use of net_lock in the local module and decouple it from
other network modules.

Signed-off-by: zhanghongyu <zhanghongyu@xiaomi.com>
@github-actions github-actions bot added Area: Networking Effects networking subsystem Size: M The size of the change in this PR is medium labels Dec 30, 2025
Copy link
Contributor

@anchao anchao left a comment

Choose a reason for hiding this comment

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

Why not reuse net_lock and extended it to support lock instances? if in this way, there's no need to add unlock/lock operations in each branch, which can avoid lock imbalance issues and eliminate the need to worry about whether to call lock or unlock at the code level.

@zhhyu7
Copy link
Contributor Author

zhhyu7 commented Dec 30, 2025

Why not reuse net_lock and extended it to support lock instances? if in this way, there's no need to add unlock/lock operations in each branch, which can avoid lock imbalance issues and eliminate the need to worry about whether to call lock or unlock at the code level.

@anchao If we directly modify the net_lock prototype, too much code needs to be changed, moreover, it's possible that some net_lock in external repository drivers may be affected, leading to compilation errors. Therefore, each relatively independent protocol has added its own global lock. I will submit a patch for code format optimization later, changing all device-related protocol code to a format similar to the previous net_xxx_wait. For local socket, I can also add a unified interface with parameters and breaklock to optimize the code in the unlock/wait/lock format. Do you think this is reasonable?
https://github.com/apache/nuttx/pull/17734/files#diff-85924974029d3e344dd6da506275a7cc78cde793d529d8336bed944144beda55R494

@xiaoxiang781216
Copy link
Contributor

Why not reuse net_lock and extended it to support lock instances? if in this way, there's no need to add unlock/lock operations in each branch, which can avoid lock imbalance issues and eliminate the need to worry about whether to call lock or unlock at the code level.

net_lock/net_unlock need keep the prototype without change since many net driver call net_lock/net_unlock and we don't have any plan to change them.

@anchao
Copy link
Contributor

anchao commented Dec 31, 2025

@xiaoxiang781216 @zhhyu7
Alternatively, we can add a dedicated set of APIs for the new local locks while keeping the prototype of net_lock unchanged.

extension net/utils/net_lock.c -> net/utils/net_lock_ext.c

net_lock/net_unlock need keep the prototype without change since many net driver call net_lock/net_unlock and we don't have any plan to change them.

I have another question, if net_lock in the board code is not properly adjusted, wouldn't this lead to deadlocks or race conditions? This is because after the modification, there will be 2 separate locks (one for the protocol stack and one for the board), and I am concerned that some callback strategies may result in ABBA dead lock

@zhhyu7
Copy link
Contributor Author

zhhyu7 commented Dec 31, 2025

@xiaoxiang781216 @zhhyu7 Alternatively, we can add a dedicated set of APIs for the new local locks while keeping the prototype of net_lock unchanged.

extension net/utils/net_lock.c -> net/utils/net_lock_ext.c

net_lock/net_unlock need keep the prototype without change since many net driver call net_lock/net_unlock and we don't have any plan to change them.

I have another question, if net_lock in the board code is not properly adjusted, wouldn't this lead to deadlocks or race conditions? This is because after the modification, there will be 2 separate locks (one for the protocol stack and one for the board), and I am concerned that some callback strategies may result in ABBA dead lock

@anchao Yes, If there is a callback strategy that may lead to ABBA deadlock, the corresponding driver will need to make relevant adaptations. Currently, we can only ensure that processes that net_lock is not first attempted to be acquired in the callback is compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Networking Effects networking subsystem Size: M The size of the change in this PR is medium

3 participants