Skip to content

Conversation

miri64
Copy link
Member

@miri64 miri64 commented May 3, 2017

Reinstating #6294.

@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation GNRC labels May 3, 2017
@miri64 miri64 requested a review from PeterKietzmann May 3, 2017 12:44
@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 3, 2017
@miri64
Copy link
Member Author

miri64 commented May 3, 2017

Tested it with various packet formats and sizes in gnrc_networking on samr21-xpro and the following patch to be able to check for memory leaks:

diff --git a/examples/gnrc_networking/main.c b/examples/gnrc_networking/main.c
index 6301f42..52637ca 100644
--- a/examples/gnrc_networking/main.c
+++ b/examples/gnrc_networking/main.c
@@ -22,14 +22,24 @@
 
 #include "shell.h"
 #include "msg.h"
+#include "net/gnrc/pktbuf.h"
 
 #define MAIN_QUEUE_SIZE     (8)
 static msg_t _main_msg_queue[MAIN_QUEUE_SIZE];
 
 extern int udp_cmd(int argc, char **argv);
 
+static int pktbuf_cmd(int argc, char **argv)
+{
+    (void)argc;
+    (void)argv;
+    gnrc_pktbuf_stats();
+    return 0;
+}
+
 static const shell_command_t shell_commands[] = {
     { "udp", "send data over UDP and listen on UDP ports", udp_cmd },
+    { "pktbuf", "---", pktbuf_cmd },
     { NULL, NULL, NULL }
 };
 

All seems well.

@PeterKietzmann
Copy link
Member

PeterKietzmann commented Jun 12, 2017

I don't see the 'big benefit' behind this but I can confirm it improves performance by about 1-1,5% (evaluated with my l2 reflector). I tested with a UDP application sending packets with payloads: 0:1:1210 Bytes. Additionally I tried ping6 with gnrc_networking and different payloads (10, 100, 500, 1210 Bytes). All went well. Pleas rebase

@smlng
Copy link
Member

smlng commented Jun 19, 2017

I don't see the benefit of the realloc in general, either. I mean, the most expensive (and sometimes problematic) part is the initial read without a buffer to get bytes_expected.

How large can the error (size diff) be for nbytes <= bytes_expected, anyway? Hence, it might even be worth considering to remove the realloc completely to save some code and checks, and live with the marginal error of wasting some bytes in the packet buffer.

Otherwise one could argue, that we could save the first read to get bytes_expected and simply always realloc.

@PeterKietzmann
Copy link
Member

@miri64 do you personally think this change is useful? It mainly changes your own code.

@miri64
Copy link
Member Author

miri64 commented Sep 4, 2017

@PeterKietzmann wasn't this based on your recommendation, that realloc is causing timing problems (at least from what I remember). As such, I think this is useful, yes.

@miri64
Copy link
Member Author

miri64 commented Nov 6, 2017

Ping @PeterKietzmann?

@PeterKietzmann
Copy link
Member

This did not fix the actual performance regression back then. But at least it improved processing at about ~1% or so. Anyway, both @smlng and me seem to be unconfined about the usefulness of this PR. That's why I'm asking for your personal opinion, since you wrote the initial code. I won't NACK this PR. So in case you are in favor of this PR, please rebase.

@miri64
Copy link
Member Author

miri64 commented Nov 16, 2017

The code was originally by @kaspar030, but I think it is useful, as it frees wasted packet buffer space as soon as possible. As to if we could just remove the gnrc_pktbuf_realloc() as proposed by @smlng: might be, but we should evaluate this first.

I would need to redo this PR for gnrc_netif2 anyway, so if you don't think it is useful and since no-one seems to be interested in this at the moment, I close it as memo.

@miri64 miri64 closed this Nov 16, 2017
@miri64 miri64 added the State: archived State: The PR has been archived for possible future re-adaptation label Nov 16, 2017
@miri64 miri64 deleted the gnrc_netdev_ieee802154/fix/port-6294 branch November 16, 2017 12:55
@miri64 miri64 added Platform: MSP Platform: This PR/issue effects MSP-based platforms Area: network Area: Networking labels Sep 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: MSP Platform: This PR/issue effects MSP-based platforms State: archived State: The PR has been archived for possible future re-adaptation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants