Skip to content

TCP endpoint compare refactoring #24407

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

Merged
merged 2 commits into from
Apr 19, 2020
Merged

TCP endpoint compare refactoring #24407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
46106ec
net: tcp2: Avoid extra memory allocation in endpoint cmp
jukkar Apr 16, 2020
3bc67f0
tests: net: tcp2: Add more debugging info
jukkar Apr 17, 2020
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
87 changes: 56 additions & 31 deletions subsys/net/ip/tcp2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,38 +58,57 @@ static size_t tcp_endpoint_len(sa_family_t af)
sizeof(struct sockaddr_in6);
}

static union tcp_endpoint *tcp_endpoint_new(struct net_pkt *pkt, int src)
static union tcp_endpoint *tcp_endpoint_new(struct net_pkt *pkt)
{
sa_family_t af = net_pkt_family(pkt);
union tcp_endpoint *ep = tcp_calloc(1, tcp_endpoint_len(af));

ep->sa.sa_family = af;
if (ep) {
ep->sa.sa_family = af;
}

switch (af) {
case AF_INET: {
struct net_ipv4_hdr *ip = (struct net_ipv4_hdr *)
net_pkt_ip_data(pkt);
struct tcphdr *th = th_get(pkt);
return ep;
}

ep->sin.sin_port = src ? th->th_sport : th->th_dport;
static union tcp_endpoint *tcp_endpoint_set(union tcp_endpoint *ep,
Copy link
Contributor

@ozhuraki ozhuraki Apr 17, 2020

Choose a reason for hiding this comment

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

I would suggest to change the return type to bool. This way it would be easier to do the error handling more clear and intuitive inside this function and in places of its usage.

Copy link
Member Author

@jukkar jukkar Apr 17, 2020

Choose a reason for hiding this comment

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

I did not want to change the current behaviour too much. So new tcp_endpoint_set() can be put to same place where tcp_endpoint_new() was earlier. Actually we do not seem to handle the out-of-mem case very well in current code either. Hmm, I will send another PR that adds more error checks, will check also would it be better to change the return type.

Copy link
Contributor

@ozhuraki ozhuraki Apr 17, 2020

Choose a reason for hiding this comment

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

Sure, this can be done later. These (handling out or memory and more explicit/clear error handling) were my points.

Alternative approach to consider later is just to embed endpoints into struct tcp and to drop the heap allocations.

Copy link
Member Author

@jukkar jukkar Apr 19, 2020

Choose a reason for hiding this comment

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

Alternative approach to consider later is just to embed endpoints into struct tcp and to drop the heap allocations.

Yes, that would be better way indeed.

struct net_pkt *pkt,
enum pkt_addr src)
{
if (!ep) {
return NULL;
}

ep->sin.sin_addr = src ? ip->src : ip->dst;
switch (ep->sa.sa_family) {
case AF_INET:
if (IS_ENABLED(CONFIG_NET_IPV4)) {
struct net_ipv4_hdr *ip = (struct net_ipv4_hdr *)
net_pkt_ip_data(pkt);
struct tcphdr *th = th_get(pkt);

ep->sin.sin_port = src == TCP_EP_SRC ? th->th_sport :
th->th_dport;
ep->sin.sin_addr = src == TCP_EP_SRC ? ip->src :
ip->dst;
}

break;
}
case AF_INET6: {
struct net_ipv6_hdr *ip = (struct net_ipv6_hdr *)
net_pkt_ip_data(pkt);
struct tcphdr *th = th_get(pkt);

ep->sin6.sin6_port = src ? th->th_sport : th->th_dport;

ep->sin6.sin6_addr = src ? ip->src : ip->dst;
case AF_INET6:
if (IS_ENABLED(CONFIG_NET_IPV6)) {
struct net_ipv6_hdr *ip = (struct net_ipv6_hdr *)
net_pkt_ip_data(pkt);
struct tcphdr *th = th_get(pkt);

ep->sin6.sin6_port = src == TCP_EP_SRC ? th->th_sport :
th->th_dport;
ep->sin6.sin6_addr = src == TCP_EP_SRC ? ip->src :
ip->dst;
}

break;
}

default:
NET_ERR("Unknown address family: %hu", af);
NET_ERR("Unknown address family: %hu", ep->sa.sa_family);
}

return ep;
Expand Down Expand Up @@ -689,15 +708,15 @@ int net_tcp_get(struct net_context *context)
}

static bool tcp_endpoint_cmp(union tcp_endpoint *ep, struct net_pkt *pkt,
int which)
enum pkt_addr which)
{
union tcp_endpoint *ep_new = tcp_endpoint_new(pkt, which);
bool is_equal = memcmp(ep, ep_new, tcp_endpoint_len(ep->sa.sa_family)) ?
false : true;
union tcp_endpoint ep_tmp = { 0 }, *ep_ptr;
Copy link
Contributor

@ozhuraki ozhuraki Apr 17, 2020

Choose a reason for hiding this comment

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

Perhaps, ep_ptr is extra here, &ep_tmp can be passed right to memcmp().


ep_tmp.sa.sa_family = net_pkt_family(pkt);

tcp_free(ep_new);
ep_ptr = tcp_endpoint_set(&ep_tmp, pkt, which);

return is_equal;
return !memcmp(ep, ep_ptr, tcp_endpoint_len(ep->sa.sa_family));
}

static bool tcp_conn_cmp(struct tcp *conn, struct net_pkt *pkt)
Expand Down Expand Up @@ -789,8 +808,8 @@ static struct tcp *tcp_conn_new(struct net_pkt *pkt)

net_context_set_family(conn->context, pkt->family);

conn->dst = tcp_endpoint_new(pkt, TCP_EP_SRC);
conn->src = tcp_endpoint_new(pkt, TCP_EP_DST);
conn->dst = tcp_endpoint_set(tcp_endpoint_new(pkt), pkt, TCP_EP_SRC);
Copy link
Contributor

@ozhuraki ozhuraki Apr 17, 2020

Choose a reason for hiding this comment

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

Per your judgement, consider the first comment (tcp_enpoint_set() -> bool), might make the error/reporting handling more clear.

conn->src = tcp_endpoint_set(tcp_endpoint_new(pkt), pkt, TCP_EP_DST);

NET_DBG("conn: src: %s, dst: %s",
log_strdup(tcp_endpoint_to_string(conn->src)),
Expand Down Expand Up @@ -1259,8 +1278,10 @@ static enum net_verdict tcp_input(struct net_conn *net_conn,
net_tcp_get(context);
net_context_set_family(context, pkt->family);
conn = context->tcp;
conn->dst = tcp_endpoint_new(pkt, TCP_EP_SRC);
conn->src = tcp_endpoint_new(pkt, TCP_EP_DST);
conn->dst = tcp_endpoint_set(tcp_endpoint_new(pkt),
pkt, TCP_EP_SRC);
conn->src = tcp_endpoint_set(tcp_endpoint_new(pkt),
pkt, TCP_EP_DST);
/* Make an extra reference, the sanity check suite
* will delete the connection explicitly
*/
Expand Down Expand Up @@ -1379,8 +1400,12 @@ enum net_verdict tp_input(struct net_conn *net_conn,
net_tcp_get(context);
net_context_set_family(context, pkt->family);
conn = context->tcp;
conn->dst = tcp_endpoint_new(pkt, TCP_EP_SRC);
conn->src = tcp_endpoint_new(pkt, TCP_EP_DST);
conn->dst =
tcp_endpoint_set(tcp_endpoint_new(pkt),
pkt, TCP_EP_SRC);
conn->src =
tcp_endpoint_set(tcp_endpoint_new(pkt),
pkt, TCP_EP_DST);
conn->iface = pkt->iface;
tcp_conn_ref(conn);
}
Expand Down
24 changes: 12 additions & 12 deletions tests/net/tcp2/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,13 +149,13 @@ static void test_sem_give(void)
k_sem_give(&test_sem);
}

static void test_sem_take(k_timeout_t timeout)
static void test_sem_take(k_timeout_t timeout, int line)
{
sem = true;
k_sem_take(&test_sem, timeout);

if (sem) {
zassert_true(false, "semaphore timed out");
zassert_true(false, "semaphore timed out (line %d)", line);
}
}

Expand Down Expand Up @@ -467,22 +467,22 @@ static void test_client_ipv4(void)
/* Peer will release the semaphone after it receives
* proper ACK to SYN | ACK
*/
test_sem_take(K_MSEC(100));
test_sem_take(K_MSEC(100), __LINE__);

ret = net_context_send(ctx, &data, 1, NULL, K_NO_WAIT, NULL);
if (ret < 0) {
zassert_true(false, "Failed to send data to peer");
}

/* Peer will release the semaphone after it sends ACK for data */
test_sem_take(K_MSEC(100));
test_sem_take(K_MSEC(100), __LINE__);

net_tcp_put(ctx);

/* Peer will release the semaphone after it receives
* proper ACK to FIN | ACK
*/
test_sem_take(K_MSEC(100));
test_sem_take(K_MSEC(100), __LINE__);
}

/* Test case scenario IPv6
Expand Down Expand Up @@ -522,22 +522,22 @@ static void test_client_ipv6(void)
/* Peer will release the semaphone after it receives
* proper ACK to SYN | ACK
*/
test_sem_take(K_MSEC(100));
test_sem_take(K_MSEC(100), __LINE__);

ret = net_context_send(ctx, &data, 1, NULL, K_NO_WAIT, NULL);
if (ret < 0) {
zassert_true(false, "Failed to send data to peer");
}

/* Peer will release the semaphone after it sends ACK for data */
test_sem_take(K_MSEC(100));
test_sem_take(K_MSEC(100), __LINE__);

net_tcp_put(ctx);

/* Peer will release the semaphone after it receives
* proper ACK to FIN | ACK
*/
test_sem_take(K_MSEC(100));
test_sem_take(K_MSEC(100), __LINE__);
}

static void handle_server_test(sa_family_t af, struct tcphdr *th)
Expand Down Expand Up @@ -681,7 +681,7 @@ static void test_server_ipv4(void)
/* test_tcp_accept_cb will release the semaphone after succesfull
* connection.
*/
test_sem_take(K_MSEC(100));
test_sem_take(K_MSEC(100), __LINE__);

/* Trigger the peer to send DATA */
k_delayed_work_submit(&test_server, K_NO_WAIT);
Expand Down Expand Up @@ -744,7 +744,7 @@ static void test_server_with_options_ipv4(void)
/* test_tcp_accept_cb will release the semaphone after succesfull
* connection.
*/
test_sem_take(K_MSEC(100));
test_sem_take(K_MSEC(100), __LINE__);

/* Trigger the peer to send DATA */
k_delayed_work_submit(&test_server, K_NO_WAIT);
Expand Down Expand Up @@ -807,7 +807,7 @@ static void test_server_ipv6(void)
/* test_tcp_accept_cb will release the semaphone after succesfull
* connection.
*/
test_sem_take(K_MSEC(100));
test_sem_take(K_MSEC(100), __LINE__);

/* Trigger the peer to send DATA */
k_delayed_work_submit(&test_server, K_NO_WAIT);
Expand Down Expand Up @@ -863,7 +863,7 @@ static void test_client_syn_resend(void)
}

/* test handler will release the sem once it receives SYN again */
test_sem_take(K_MSEC(500));
test_sem_take(K_MSEC(500), __LINE__);

net_context_put(ctx);
}
Expand Down