From: Pablo Neira Ayuso pablo@gnumonks.org
Hi,
These patchset contains the port of libctrl to use the IPA infrastructure available in libosmo-abis.
I've also made one small cleanup to bail out in case that we cannot bind to the telnet port. Another patch follows up to avoid disabling nagle (to avoid problems with TCP segmentation, we still have to support this appropriately), I noticed this while doing the port work.
Pablo Neira Ayuso (3): ctrl: use generic IPA socket infrastructure available in libosmo-abis src: check for error returned by controlif_setup() ctrl: do not disable nagle algorithm until we fully support segmentation
openbsc/include/openbsc/control_cmd.h | 17 +--- openbsc/include/openbsc/control_if.h | 2 +- openbsc/src/libctrl/control_if.c | 153 +++++++-------------------------- openbsc/src/osmo-bsc/osmo_bsc_main.c | 6 +- openbsc/src/osmo-bsc_nat/bsc_nat.c | 6 +- openbsc/src/osmo-nitb/bsc_hack.c | 5 +- 6 files changed, 50 insertions(+), 139 deletions(-)
From: Pablo Neira Ayuso pablo@gnumonks.org
This patch puts libctrl into diet by using the generic IPA socket infrastructure available in libosmo-abis.
We save 93 lines of code with this and we remove the use of the deprecated socket infrastructure like make_sock(...). --- openbsc/include/openbsc/control_cmd.h | 17 +---- openbsc/include/openbsc/control_if.h | 2 +- openbsc/src/libctrl/control_if.c | 148 ++++++++------------------------- 3 files changed, 37 insertions(+), 130 deletions(-)
diff --git a/openbsc/include/openbsc/control_cmd.h b/openbsc/include/openbsc/control_cmd.h index 2a5391f..87db33f 100644 --- a/openbsc/include/openbsc/control_cmd.h +++ b/openbsc/include/openbsc/control_cmd.h @@ -29,21 +29,8 @@ enum ctrl_type { CTRL_TYPE_ERROR };
-struct ctrl_connection { - struct llist_head list_entry; - - /* The queue for sending data back */ - struct osmo_wqueue write_queue; - - /* Callback if the connection was closed */ - void (*closed_cb)(struct ctrl_connection *conn); - - /* Pending commands for this connection */ - struct llist_head cmds; -}; - struct ctrl_cmd { - struct ctrl_connection *ccon; + struct ipa_server_conn *ipa_link; enum ctrl_type type; char *id; void *node; @@ -74,7 +61,7 @@ struct ctrl_cmd_map { int ctrl_cmd_exec(vector vline, struct ctrl_cmd *command, vector node, void *data); int ctrl_cmd_install(enum ctrl_node_type node, struct ctrl_cmd_element *cmd); int ctrl_cmd_handle(struct ctrl_cmd *cmd, void *data); -int ctrl_cmd_send(struct osmo_wqueue *queue, struct ctrl_cmd *cmd); +int ctrl_cmd_send(struct ipa_server_conn *ipa_server_conn, struct ctrl_cmd *cmd); struct ctrl_cmd *ctrl_cmd_parse(void *ctx, struct msgb *msg); struct msgb *ctrl_cmd_make(struct ctrl_cmd *cmd); struct ctrl_cmd *ctrl_cmd_cpy(void *ctx, struct ctrl_cmd *cmd); diff --git a/openbsc/include/openbsc/control_if.h b/openbsc/include/openbsc/control_if.h index 96fbf6b..50ab962 100644 --- a/openbsc/include/openbsc/control_if.h +++ b/openbsc/include/openbsc/control_if.h @@ -5,7 +5,7 @@ #include <openbsc/control_cmd.h> #include <openbsc/gsm_data.h>
-int ctrl_cmd_send(struct osmo_wqueue *queue, struct ctrl_cmd *cmd); +int ctrl_cmd_send(struct ipa_server_conn *ipa_server_conn, struct ctrl_cmd *cmd); int ctrl_cmd_handle(struct ctrl_cmd *cmd, void *data); int controlif_setup(struct gsm_network *gsmnet, uint16_t port);
diff --git a/openbsc/src/libctrl/control_if.c b/openbsc/src/libctrl/control_if.c index 8198ae6..58476ec 100644 --- a/openbsc/src/libctrl/control_if.c +++ b/openbsc/src/libctrl/control_if.c @@ -63,15 +63,14 @@ #include <osmocom/abis/ipa.h>
struct ctrl_handle { - struct osmo_fd listen_fd; + struct ipa_server_link *ipa_link; struct gsm_network *gsmnet; };
vector ctrl_node_vec;
-int ctrl_cmd_send(struct osmo_wqueue *queue, struct ctrl_cmd *cmd) +int ctrl_cmd_send(struct ipa_server_conn *ipa_server_conn, struct ctrl_cmd *cmd) { - int ret; struct msgb *msg;
msg = ctrl_cmd_make(cmd); @@ -83,12 +82,8 @@ int ctrl_cmd_send(struct osmo_wqueue *queue, struct ctrl_cmd *cmd) ipaccess_prepend_header_ext(msg, IPAC_PROTO_EXT_CTRL); ipaccess_prepend_header(msg, IPAC_PROTO_OSMO);
- ret = osmo_wqueue_enqueue(queue, msg); - if (ret != 0) { - LOGP(DCTRL, LOGL_ERROR, "Failed to enqueue the command.\n"); - msgb_free(msg); - } - return ret; + ipa_server_conn_send(ipa_server_conn, msg); + return 0; }
int ctrl_cmd_handle(struct ctrl_cmd *cmd, void *data) @@ -192,43 +187,14 @@ err: return ret; }
-static void control_close_conn(struct ctrl_connection *ccon) -{ - close(ccon->write_queue.bfd.fd); - osmo_fd_unregister(&ccon->write_queue.bfd); - if (ccon->closed_cb) - ccon->closed_cb(ccon); - talloc_free(ccon); -} - -static int handle_control_read(struct osmo_fd * bfd) +static int +handle_control_read(struct ipa_server_conn *ipa_server_conn, struct msgb *msg) { int ret = -1; - struct osmo_wqueue *queue; - struct ctrl_connection *ccon; struct ipaccess_head *iph; struct ipaccess_head_ext *iph_ext; - struct msgb *msg; struct ctrl_cmd *cmd; - struct ctrl_handle *ctrl = bfd->data; - - queue = container_of(bfd, struct osmo_wqueue, bfd); - ccon = container_of(queue, struct ctrl_connection, write_queue); - - ret = ipa_msg_recv(bfd->fd, &msg); - if (ret <= 0) { - if (ret == 0) - LOGP(DCTRL, LOGL_INFO, "The control connection was closed\n"); - else - LOGP(DCTRL, LOGL_ERROR, "Failed to parse ip access message: %d\n", ret); - - goto err; - } - - if (msg->len < sizeof(*iph) + sizeof(*iph_ext)) { - LOGP(DCTRL, LOGL_ERROR, "The message is too short.\n"); - goto err; - } + struct ctrl_handle *ctrl = ipa_server_conn->data;
iph = (struct ipaccess_head *) msg->data; if (iph->proto != IPAC_PROTO_OSMO) { @@ -244,23 +210,23 @@ static int handle_control_read(struct osmo_fd * bfd)
msg->l2h = iph_ext->data;
- cmd = ctrl_cmd_parse(ccon, msg); + cmd = ctrl_cmd_parse(ipa_server_conn, msg);
if (cmd) { - cmd->ccon = ccon; + cmd->ipa_link = ipa_server_conn; if (ctrl_cmd_handle(cmd, ctrl->gsmnet) != CTRL_CMD_HANDLED) { - ctrl_cmd_send(queue, cmd); + ctrl_cmd_send(ipa_server_conn, cmd); talloc_free(cmd); } } else { - cmd = talloc_zero(ccon, struct ctrl_cmd); + cmd = talloc_zero(ipa_server_conn, struct ctrl_cmd); if (!cmd) goto err; LOGP(DCTRL, LOGL_ERROR, "Command parser error.\n"); cmd->type = CTRL_TYPE_ERROR; cmd->id = "err"; cmd->reply = "Command parser error."; - ctrl_cmd_send(queue, cmd); + ctrl_cmd_send(ipa_server_conn, cmd); talloc_free(cmd); }
@@ -268,82 +234,31 @@ static int handle_control_read(struct osmo_fd * bfd) return 0;
err: - control_close_conn(ccon); + ipa_server_conn_destroy(ipa_server_conn); msgb_free(msg); return ret; }
-static int control_write_cb(struct osmo_fd *bfd, struct msgb *msg) -{ - int rc; - - rc = write(bfd->fd, msg->data, msg->len); - if (rc != msg->len) - LOGP(DCTRL, LOGL_ERROR, "Failed to write message to the control connection.\n"); - - return rc; -} - -static struct ctrl_connection *ctrl_connection_alloc(void *ctx) -{ - struct ctrl_connection *ccon = talloc_zero(ctx, struct ctrl_connection); - if (!ccon) - return NULL; - - osmo_wqueue_init(&ccon->write_queue, 100); - /* Error handling here? */ - - INIT_LLIST_HEAD(&ccon->cmds); - return ccon; -} - -static int listen_fd_cb(struct osmo_fd *listen_bfd, unsigned int what) +static int ctrl_accept_cb(struct ipa_server_link *ipa_link, int fd) { - int ret, fd, on; - struct ctrl_connection *ccon; - struct sockaddr_in sa; - socklen_t sa_len = sizeof(sa); - - - if (!(what & BSC_FD_READ)) - return 0; - - fd = accept(listen_bfd->fd, (struct sockaddr *) &sa, &sa_len); - if (fd < 0) { - perror("accept"); - return fd; - } - LOGP(DCTRL, LOGL_INFO, "accept()ed new control connection from %s\n", - inet_ntoa(sa.sin_addr)); + int ret, on = 1; + struct ipa_server_conn *ipa_peer_link;
- on = 1; ret = setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, &on, sizeof(on)); if (ret != 0) { LOGP(DNAT, LOGL_ERROR, "Failed to set TCP_NODELAY: %s\n", strerror(errno)); close(fd); return ret; } - ccon = ctrl_connection_alloc(listen_bfd->data); - if (!ccon) { - LOGP(DCTRL, LOGL_ERROR, "Failed to allocate.\n"); - close(fd); - return -1; - } - - ccon->write_queue.bfd.data = listen_bfd->data; - ccon->write_queue.bfd.fd = fd; - ccon->write_queue.bfd.when = BSC_FD_READ; - ccon->write_queue.read_cb = handle_control_read; - ccon->write_queue.write_cb = control_write_cb; - - ret = osmo_fd_register(&ccon->write_queue.bfd); - if (ret < 0) { - LOGP(DCTRL, LOGL_ERROR, "Could not register FD.\n"); - close(ccon->write_queue.bfd.fd); - talloc_free(ccon); + ipa_peer_link = ipa_server_conn_create(tall_bsc_ctx, ipa_link, fd, + handle_control_read, + ipa_link->data); + if (!ipa_peer_link) { + LOGP(DCTRL, LOGL_ERROR, "Failed to register peer connection.\n"); + return -ENOMEM; }
- return ret; + return 0; }
static uint64_t get_rate_ctr_value(const struct rate_ctr *ctr, int intv) @@ -601,7 +516,6 @@ static int verify_counter(struct ctrl_cmd *cmd, const char *value, void *data)
int controlif_setup(struct gsm_network *gsmnet, uint16_t port) { - int ret; struct ctrl_handle *ctrl;
ctrl = talloc_zero(tall_bsc_ctx, struct ctrl_handle); @@ -615,15 +529,21 @@ int controlif_setup(struct gsm_network *gsmnet, uint16_t port) return -ENOMEM;
/* Listen for control connections */ - ret = make_sock(&ctrl->listen_fd, IPPROTO_TCP, INADDR_LOOPBACK, port, - 0, listen_fd_cb, ctrl); - if (ret < 0) { + ctrl->ipa_link = ipa_server_link_create(ctrl, NULL, + "127.0.0.1", port, + ctrl_accept_cb, ctrl); + if (!ctrl->ipa_link) { talloc_free(ctrl); - return ret; + return -ENOMEM; + } + + if (ipa_server_link_open(ctrl->ipa_link) < 0) { + talloc_free(ctrl); + return -EIO; }
ctrl_cmd_install(CTRL_NODE_ROOT, &cmd_rate_ctr); ctrl_cmd_install(CTRL_NODE_ROOT, &cmd_counter);
- return ret; + return 0; }
From: Pablo Neira Ayuso pablo@gnumonks.org
Bail out in case that we cannot bind the telnet interface. --- openbsc/src/osmo-bsc/osmo_bsc_main.c | 6 +++++- openbsc/src/osmo-bsc_nat/bsc_nat.c | 6 +++++- openbsc/src/osmo-nitb/bsc_hack.c | 5 ++++- 3 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/openbsc/src/osmo-bsc/osmo_bsc_main.c b/openbsc/src/osmo-bsc/osmo_bsc_main.c index 9a799c0..fe6526d 100644 --- a/openbsc/src/osmo-bsc/osmo_bsc_main.c +++ b/openbsc/src/osmo-bsc/osmo_bsc_main.c @@ -422,7 +422,11 @@ int main(int argc, char **argv) } bsc_api_init(bsc_gsmnet, osmo_bsc_api());
- controlif_setup(bsc_gsmnet, 4249); + if (controlif_setup(bsc_gsmnet, 4249) < 0) { + fprintf(stderr, "CTRL: Cannot bind to port 4249\n"); + exit(1); + } + ctrl_cmd_install(CTRL_NODE_NET, &cmd_net_loc); ctrl_cmd_install(CTRL_NODE_NET, &cmd_net_rf_lock); ctrl_cmd_install(CTRL_NODE_TRX, &cmd_trx_rf_lock); diff --git a/openbsc/src/osmo-bsc_nat/bsc_nat.c b/openbsc/src/osmo-bsc_nat/bsc_nat.c index 295535a..326d04e 100644 --- a/openbsc/src/osmo-bsc_nat/bsc_nat.c +++ b/openbsc/src/osmo-bsc_nat/bsc_nat.c @@ -1760,7 +1760,11 @@ int main(int argc, char **argv) exit(1); }
- controlif_setup(NULL, 4250); + if (controlif_setup(NULL, 4250) < 0) { + fprintf(stderr, "CTRL: Cannot bind to port 4250\n"); + exit(1); + } + ctrl_cmd_install(CTRL_NODE_ROOT, &cmd_fwd_cmd);
nat->msc_con->connection_loss = msc_connection_was_lost; diff --git a/openbsc/src/osmo-nitb/bsc_hack.c b/openbsc/src/osmo-nitb/bsc_hack.c index 001d8f9..5f9b7e5 100644 --- a/openbsc/src/osmo-nitb/bsc_hack.c +++ b/openbsc/src/osmo-nitb/bsc_hack.c @@ -254,7 +254,10 @@ int main(int argc, char **argv) exit(1); bsc_api_init(bsc_gsmnet, msc_bsc_api());
- controlif_setup(bsc_gsmnet, 4249); + if (controlif_setup(bsc_gsmnet, 4249) < 0) { + fprintf(stderr, "CTRL: Cannot bind to port 4249\n"); + exit(1); + } /* seed the PRNG */ srand(time(NULL));
From: Pablo Neira Ayuso pablo@gnumonks.org
We shouldn't disable nagle, we have to finish TCP segmentation support appropriately first. --- openbsc/src/libctrl/control_if.c | 7 ------- 1 files changed, 0 insertions(+), 7 deletions(-)
diff --git a/openbsc/src/libctrl/control_if.c b/openbsc/src/libctrl/control_if.c index 58476ec..7e2ae16 100644 --- a/openbsc/src/libctrl/control_if.c +++ b/openbsc/src/libctrl/control_if.c @@ -241,15 +241,8 @@ err:
static int ctrl_accept_cb(struct ipa_server_link *ipa_link, int fd) { - int ret, on = 1; struct ipa_server_conn *ipa_peer_link;
- ret = setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, &on, sizeof(on)); - if (ret != 0) { - LOGP(DNAT, LOGL_ERROR, "Failed to set TCP_NODELAY: %s\n", strerror(errno)); - close(fd); - return ret; - } ipa_peer_link = ipa_server_conn_create(tall_bsc_ctx, ipa_link, fd, handle_control_read, ipa_link->data);
Hi Pablo,
thanks for your patches.
On Fri, Sep 09, 2011 at 03:05:29AM +0200, pablo@gnumonks.org wrote:
We shouldn't disable nagle, we have to finish TCP segmentation support appropriately first.
I would like to hear Daniel's comment on those patches, particularly on the reason he was disabling nagle...
On Fri, Sep 09, 2011 at 03:05:26AM +0200, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
Hi,
These patchset contains the port of libctrl to use the IPA infrastructure available in libosmo-abis.
I've also made one small cleanup to bail out in case that we cannot bind to the telnet port. Another patch follows up to avoid disabling nagle (to avoid problems with TCP segmentation, we still have to support this appropriately), I noticed this while doing the port work.
Pablo Neira Ayuso (3): ctrl: use generic IPA socket infrastructure available in libosmo-abis src: check for error returned by controlif_setup() ctrl: do not disable nagle algorithm until we fully support segmentation
I forgot to say that they are available in the pablo/ctrl-updates branch.
BTW, I have used the following command line interactive client to test the changes. I made it because I wanted to test the IPA socket infrastructure from the client side. Just in case you find it of any use. You can find it attached.
Pablo Neira Ayuso pablo@gnumonks.org wrote:
On Fri, Sep 09, 2011 at 03:05:26AM +0200, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
Hi,
These patchset contains the port of libctrl to use the IPA infrastructure available in libosmo-abis.
I've also made one small cleanup to bail out in case that we cannot bind to the telnet port. Another patch follows up to avoid disabling nagle (to avoid problems with TCP segmentation, we still have to support this appropriately), I noticed this while doing the port work.
Thanks Pablo, I'll take a look at them tonight. Not sure about the nodelay issue, I think it was an issue of doing it similar to other places in the code, but I'll check.
Regards, Daniel
On Fri, Sep 09, 2011 at 03:51:08PM +0200, Daniel Willmann wrote:
Pablo Neira Ayuso pablo@gnumonks.org wrote:
On Fri, Sep 09, 2011 at 03:05:26AM +0200, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
Hi,
These patchset contains the port of libctrl to use the IPA infrastructure available in libosmo-abis.
I've also made one small cleanup to bail out in case that we cannot bind to the telnet port. Another patch follows up to avoid disabling nagle (to avoid problems with TCP segmentation, we still have to support this appropriately), I noticed this while doing the port work.
Thanks Pablo, I'll take a look at them tonight. Not sure about the nodelay issue, I think it was an issue of doing it similar to other places in the code, but I'll check.
The LOGP call in the code to disable nagle was using DNAT logging level, so I guess it's a leftover. I still think we have to remove this option until we fully support TCP segmentation (I'm working on some patches for that, they will come but until that not sure what's the benefit of disabling nagle anyway).
Wait for your comments!
On 09/09/2011 03:14 AM, Pablo Neira Ayuso wrote:
I've also made one small cleanup to bail out in case that we cannot bind to the telnet port. Another patch follows up to avoid disabling nagle (to avoid problems with TCP segmentation, we still have to support this appropriately), I noticed this while doing the port work.
Hi Pablo,
besides having problems with TCP Segmentation in the reading code of the ipaccess socket the NODELAY setsockopt was added to send out messages more quickly.
holger
Hello Pablo,
sorry, but this is going to take me longer than I previously thought. Just some short remarks on what I noticed so far. After Tuesday I'll have more time on my hands to help with porting.
On Fri, 9 Sep 2011 03:05:26 +0200 pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
These patchset contains the port of libctrl to use the IPA infrastructure available in libosmo-abis.
I've also made one small cleanup to bail out in case that we cannot bind to the telnet port. Another patch follows up to avoid disabling nagle (to avoid problems with TCP segmentation, we still have to support this appropriately), I noticed this while doing the port work.
First of all thanks for starting the port. I had a brief look into that a week ago, but ran into different problems.
Pablo Neira Ayuso (3): ctrl: use generic IPA socket infrastructure available in libosmo-abis
The control commands need to be sent both over the dedicated control connection as well as between osmo-bsc and nat. It seems that the bsc_msc_connection is still using osmo_wqueues which complicates matters as I originally just used ctrl_cmd_send to send the response to whatever osmo_wqueue the command came in from or forward commands from the nat to the appropriate osmo-bsc. bsc_nat also needs to know if a a control connection is closed so it can remove any commands pending response from the pending list. The list is needed to generate errors if the msc_bsc connection is reset or a timeout occurs while a command is in transit. Could you add a callback to ipa_server_conn that gets called if the connection is closed?
src: check for error returned by controlif_setup()
That's certainly useful, thanks.
ctrl: do not disable nagle algorithm until we fully support segmentation
I had a look at commit 4462f8 and I believe I just tried to mimic the behavior of msc_connection_connected() from ./osmo-bsc/osmo_bsc_msc.c at that time.
Anyway, I'd be interested in the problems you encountered. I know that ipaccess_read_msg mishandled the case where the data read returned only a partial packet, but shouldn't that only occur if the data to be sent is larger than the current MSS? In any case I would have expected disabling the nagle algorithm to help in that regard...
So I guess there's still some work to do. I'll reserve some time on Thursday to look at the issues.
Regards, Daniel Willmann
Hi Daniel,
On Sat, Sep 10, 2011 at 05:14:58PM +0200, Daniel Willmann wrote:
Hello Pablo,
sorry, but this is going to take me longer than I previously thought.
No problem, we're not in the rush :-).
Just some short remarks on what I noticed so far. After Tuesday I'll have more time on my hands to help with porting.
On Fri, 9 Sep 2011 03:05:26 +0200 pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
These patchset contains the port of libctrl to use the IPA infrastructure available in libosmo-abis.
I've also made one small cleanup to bail out in case that we cannot bind to the telnet port. Another patch follows up to avoid disabling nagle (to avoid problems with TCP segmentation, we still have to support this appropriately), I noticed this while doing the port work.
First of all thanks for starting the port. I had a brief look into that a week ago, but ran into different problems.
Pablo Neira Ayuso (3): ctrl: use generic IPA socket infrastructure available in libosmo-abis
The control commands need to be sent both over the dedicated control connection as well as between osmo-bsc and nat. It seems that the bsc_msc_connection is still using osmo_wqueues which complicates matters as I originally just used ctrl_cmd_send to send the response to whatever osmo_wqueue the command came in from or forward commands from the nat to the appropriate osmo-bsc.
Can you come with some idea to overcome this situation? Probably we can use osmo_wqueues in the generic IPA infrastructure, let me give it some spins.
bsc_nat also needs to know if a a control connection is closed so it can remove any commands pending response from the pending list. The list is needed to generate errors if the msc_bsc connection is reset or a timeout occurs while a command is in transit. Could you add a callback to ipa_server_conn that gets called if the connection is closed?
Sure, we can add that. The idea is to make the IPA infrastructure generic enough so we can use in as many places as we can.
src: check for error returned by controlif_setup()
That's certainly useful, thanks.
ctrl: do not disable nagle algorithm until we fully support segmentation
I had a look at commit 4462f8 and I believe I just tried to mimic the behavior of msc_connection_connected() from ./osmo-bsc/osmo_bsc_msc.c at that time.
Anyway, I'd be interested in the problems you encountered. I know that ipaccess_read_msg mishandled the case where the data read returned only a partial packet, but shouldn't that only occur if the data to be sent is larger than the current MSS? In any case I would have expected disabling the nagle algorithm to help in that regard...
I think disabling nagle in that code is fine. Basically, if we disable nagle we obtain the same behaviour that in UDP sockets, which means that one send() call means one packet, and this is what we have to do by now to avoid TCP segmentation issues (that we don't handle appropriately). Drop the patch.
So I guess there's still some work to do. I'll reserve some time on Thursday to look at the issues.
Fine, count to me if I can be of any help. I'll have a look at the issues that you mentions along this week.
-
Daniel Willmann -
Harald Welte -
Holger Hans Peter Freyther -
Pablo Neira Ayuso -
pablo＠gnumonks.org