zebra: design changes in netlink batching code

Signed-off-by: Jakub Urbańczyk <xthaid@gmail.com>

diff --git a/zebra/kernel_netlink.c b/zebra/kernel_netlink.c
index 2ce48b33ef541682a0938ba93438646dddf0a064..e6a4a3d9e214d5626e712a5eb3d6da88af305062 100644 (file)
--- a/zebra/kernel_netlink.c
+++ b/zebra/kernel_netlink.c
@@ -175,10 +175,6 @@ extern struct zebra_privs_t zserv_privs;
 char nl_batch_tx_buf[NL_BATCH_TX_BUFSIZE];
 char nl_batch_rx_buf[NL_BATCH_RX_BUFSIZE];
 
-DEFINE_MTYPE_STATIC(ZEBRA, NL_BATCH_ITEM, "Netlink batch items")
-
-PREDECL_LIST(nl_batch_list)
-
 struct nl_batch {
        void *buf;
        size_t bufsiz;
@@ -189,20 +185,16 @@ struct nl_batch {
        size_t msgcnt;
 
        const struct zebra_dplane_info *zns;
-       struct nl_batch_list_head items;
-};
 
-struct nl_msg_batch_item {
-       int seq;
-       struct zebra_dplane_ctx *ctx;
-       bool ignore_res;
-       bool failure;
+       struct dplane_ctx_q ctx_list;
 
-       struct nl_batch_list_item item;
+       /*
+        * Pointer to the queue of completed contexts outbound back
+        * towards the dataplane module.
+        */
+       struct dplane_ctx_q *ctx_out_q;
 };
 
-DECLARE_LIST(nl_batch_list, struct nl_msg_batch_item, item)
-
 int netlink_talk_filter(struct nlmsghdr *h, ns_id_t ns_id, int startup)
 {
        /*
@@ -1137,17 +1129,16 @@ static int nl_batch_read_resp(struct nl_batch *bth)
        struct nlmsghdr *h;
        struct sockaddr_nl snl;
        struct msghdr msg;
-       int status;
+       int status, seq;
        const struct nlsock *nl;
-       struct nl_msg_batch_item *item, *from_item;
+       struct zebra_dplane_ctx *ctx;
+       bool ignore_msg;
 
        nl = &(bth->zns->nls);
 
        msg.msg_name = (void *)&snl;
        msg.msg_namelen = sizeof(snl);
 
-       from_item = nl_batch_list_first(&(bth->items));
-
        status = netlink_recv_msg(nl, msg, nl_batch_rx_buf,
                                  sizeof(nl_batch_rx_buf));
        if (status == -1 || status == 0)
@@ -1156,27 +1147,51 @@ static int nl_batch_read_resp(struct nl_batch *bth)
        for (h = (struct nlmsghdr *)nl_batch_rx_buf;
             (status >= 0 && NLMSG_OK(h, (unsigned int)status));
             h = NLMSG_NEXT(h, status)) {
-
+               ignore_msg = false;
+               seq = h->nlmsg_seq;
                /*
-                * Find the corresponding batch item. Received responses are in
-                * the same order as requests we sent, so we can simply iterate
-                * over the batch item list and match responses with requests
-                * at same time.
+                * Find the corresponding context object. Received responses are
+                * in the same order as requests we sent, so we can simply
+                * iterate over the context list and match responses with
+                * requests at same time.
                 */
-               frr_each_from(nl_batch_list, &(bth->items), item, from_item) {
-                       if (item->seq == (int)h->nlmsg_seq)
+               while (true) {
+                       ctx = dplane_ctx_dequeue(&(bth->ctx_list));
+                       if (ctx == NULL)
+                               break;
+
+                       dplane_ctx_enqueue_tail(bth->ctx_out_q, ctx);
+
+                       /* We have found corresponding context object. */
+                       if (dplane_ctx_get_ns(ctx)->nls.seq == seq)
                                break;
+
+                       /*
+                        * 'update' context objects take two consecutive
+                        * sequence numbers.
+                        */
+                       if (dplane_ctx_is_update(ctx)
+                           && dplane_ctx_get_ns(ctx)->nls.seq + 1 == seq) {
+                               /*
+                                * This is the situation where we get a response
+                                * to a message that should be ignored.
+                                */
+                               ignore_msg = true;
+                               break;
+                       }
                }
 
+               if (ignore_msg)
+                       continue;
+
                /*
                 * We received a message with the sequence number that isn't
                 * associated with any dplane context object.
                 */
-               if (item == NULL) {
+               if (ctx == NULL) {
                        zlog_debug(
                                "%s: skipping unassociated response, seq number %d NS %u",
                                __func__, h->nlmsg_seq, bth->zns->ns_id);
-                       from_item = nl_batch_list_first(&(bth->items));
                        continue;
                }
 
@@ -1184,7 +1199,8 @@ static int nl_batch_read_resp(struct nl_batch *bth)
                        int err = netlink_parse_error(nl, h, bth->zns, 0);
 
                        if (err == -1)
-                               item->failure = true;
+                               dplane_ctx_set_status(
+                                       ctx, ZEBRA_DPLANE_REQUEST_FAILURE);
 
                        zlog_debug("%s: netlink error message seq=%d ",
                                   __func__, h->nlmsg_seq);
@@ -1206,85 +1222,71 @@ static int nl_batch_read_resp(struct nl_batch *bth)
 
 static void nl_batch_reset(struct nl_batch *bth)
 {
-       bth->buf = nl_batch_tx_buf;
-       bth->bufsiz = sizeof(nl_batch_tx_buf);
-       bth->limit = NL_BATCH_SEND_THRESHOLD;
-
        bth->buf_head = bth->buf;
        bth->curlen = 0;
        bth->msgcnt = 0;
        bth->zns = NULL;
 
-       nl_batch_list_init(&(bth->items));
+       TAILQ_INIT(&(bth->ctx_list));
 }
 
-static void nl_batch_send(struct nl_batch *bth)
+static void nl_batch_init(struct nl_batch *bth, struct dplane_ctx_q *ctx_out_q)
 {
-       struct nl_msg_batch_item *item;
-       bool err = false;
+       bth->buf = nl_batch_tx_buf;
+       bth->bufsiz = sizeof(nl_batch_tx_buf);
+       bth->limit = NL_BATCH_SEND_THRESHOLD;
 
-       if (bth->curlen == 0 || bth->zns == NULL)
-               return;
+       bth->ctx_out_q = ctx_out_q;
 
-       if (IS_ZEBRA_DEBUG_KERNEL)
-               zlog_debug("%s: %s, batch size=%zu, msg cnt=%zu", __func__,
-                          bth->zns->nls.name, bth->curlen, bth->msgcnt);
+       nl_batch_reset(bth);
+}
+
+static void nl_batch_send(struct nl_batch *bth)
+{
+       struct zebra_dplane_ctx *ctx;
+       bool err = false;
 
-       if (netlink_send_msg(&(bth->zns->nls), bth->buf, bth->curlen) == -1)
-               err = true;
+       if (bth->curlen != 0 && bth->zns != NULL) {
+               if (IS_ZEBRA_DEBUG_KERNEL)
+                       zlog_debug("%s: %s, batch size=%zu, msg cnt=%zu",
+                                  __func__, bth->zns->nls.name, bth->curlen,
+                                  bth->msgcnt);
 
-       if (!err) {
-               if (nl_batch_read_resp(bth) == -1)
+               if (netlink_send_msg(&(bth->zns->nls), bth->buf, bth->curlen)
+                   == -1)
                        err = true;
-       }
 
-       frr_each_safe(nl_batch_list, &(bth->items), item) {
-               enum zebra_dplane_result res = ZEBRA_DPLANE_REQUEST_SUCCESS;
+               if (!err) {
+                       if (nl_batch_read_resp(bth) == -1)
+                               err = true;
+               }
+       }
 
-               /*
-                * If either sending or receiving a message batch has ended with
-                * the error, mark all dplane requests as failed.
-                */
-               if (item->failure || err)
-                       res = ZEBRA_DPLANE_REQUEST_FAILURE;
+       /* Move remaining contexts to the outbound queue. */
+       while (true) {
+               ctx = dplane_ctx_dequeue(&(bth->ctx_list));
+               if (ctx == NULL)
+                       break;
 
-               if (!item->ignore_res)
-                       dplane_ctx_set_status(item->ctx, res);
+               if (err)
+                       dplane_ctx_set_status(ctx,
+                                             ZEBRA_DPLANE_REQUEST_FAILURE);
 
-               nl_batch_list_del(&(bth->items), item);
-               XFREE(MTYPE_NL_BATCH_ITEM, item);
+               dplane_ctx_enqueue_tail(bth->ctx_out_q, ctx);
        }
 
        nl_batch_reset(bth);
 }
 
-static void nl_batch_add_item(struct nl_batch *bth, int seq,
-                             struct zebra_dplane_ctx *ctx, bool ignore_res)
-{
-       struct nl_msg_batch_item *item =
-               XCALLOC(MTYPE_NL_BATCH_ITEM, sizeof(*item));
-
-       item->seq = seq;
-       item->ctx = ctx;
-       item->ignore_res = ignore_res;
-       item->failure = false;
-
-       nl_batch_list_add_tail(&(bth->items), item);
-}
-
 enum netlink_msg_status netlink_batch_add_msg(
        struct nl_batch *bth, struct zebra_dplane_ctx *ctx,
        ssize_t (*msg_encoder)(struct zebra_dplane_ctx *, void *, size_t),
-       bool extra_msg)
+       bool ignore_res)
 {
        int seq;
        ssize_t size;
        struct nlmsghdr *msgh;
 
-       if (bth->zns != NULL
-           && bth->zns->ns_id != dplane_ctx_get_ns(ctx)->ns_id)
-               nl_batch_send(bth);
-
        size = (*msg_encoder)(ctx, bth->buf_head, bth->bufsiz - bth->curlen);
 
        /*
@@ -1311,23 +1313,18 @@ enum netlink_msg_status netlink_batch_add_msg(
        }
 
        seq = dplane_ctx_get_ns(ctx)->nls.seq;
-       if (extra_msg)
+       if (ignore_res)
                seq++;
 
        msgh = (struct nlmsghdr *)bth->buf_head;
        msgh->nlmsg_seq = seq;
        msgh->nlmsg_pid = dplane_ctx_get_ns(ctx)->nls.snl.nl_pid;
 
-       nl_batch_add_item(bth, seq, ctx, extra_msg);
-
        bth->zns = dplane_ctx_get_ns(ctx);
        bth->buf_head = ((char *)bth->buf_head) + size;
        bth->curlen += size;
        bth->msgcnt++;
 
-       if (bth->curlen > bth->limit)
-               nl_batch_send(bth);
-
        return FRR_NETLINK_QUEUED;
 }
 
@@ -1398,29 +1395,33 @@ void kernel_update_multi(struct dplane_ctx_q *ctx_list)
        struct dplane_ctx_q handled_list;
        enum netlink_msg_status res;
 
-       nl_batch_reset(&batch);
        TAILQ_INIT(&handled_list);
+       nl_batch_init(&batch, &handled_list);
 
        while (true) {
                ctx = dplane_ctx_dequeue(ctx_list);
                if (ctx == NULL)
                        break;
 
-               res = nl_put_msg(&batch, ctx);
+               if (batch.zns != NULL
+                   && batch.zns->ns_id != dplane_ctx_get_ns(ctx)->ns_id)
+                       nl_batch_send(&batch);
 
                /*
-                * If we already know the result, we set the status of the
-                * dataplane context object. Otherwise, it will be set after we
-                * send the batch.
+                * Assume all messages will succeed and then mark only the ones
+                * that failed.
                 */
-               if (res == FRR_NETLINK_SUCCESS)
-                       dplane_ctx_set_status(ctx,
-                                             ZEBRA_DPLANE_REQUEST_SUCCESS);
-               else if (res == FRR_NETLINK_ERROR)
+               dplane_ctx_set_status(ctx, ZEBRA_DPLANE_REQUEST_SUCCESS);
+
+               res = nl_put_msg(&batch, ctx);
+
+               dplane_ctx_enqueue_tail(&(batch.ctx_list), ctx);
+               if (res == FRR_NETLINK_ERROR)
                        dplane_ctx_set_status(ctx,
                                              ZEBRA_DPLANE_REQUEST_FAILURE);
 
-               dplane_ctx_enqueue_tail(&handled_list, ctx);
+               if (batch.curlen > batch.limit)
+                       nl_batch_send(&batch);
        }
 
        nl_batch_send(&batch);

diff --git a/zebra/kernel_netlink.h b/zebra/kernel_netlink.h
index 6627462c577f4c8692cabd96c26769095b24b322..7f66f2b28c2f3fca621ad34eb8622c362a9fd321 100644 (file)
--- a/zebra/kernel_netlink.h
+++ b/zebra/kernel_netlink.h
@@ -118,16 +118,16 @@ struct nl_batch;
  *               pointer to a buffer and buffer's length as parameters
  *               and should return -1 on error, 0 on buffer overflow or
  *               size of the encoded message.
- * @extra_msg:   In some cases there are two netlink messages for single
- *               context object that need to be handled differently. This flag
- *               distinguishes those.
+ * @ignore_res:  Whether the result of this message should be ignored.
+ *               This should be used in some 'update' cases where we
+ *               need to send two messages for one context object.
  *
  * Return:             Status of the message.
  */
 extern enum netlink_msg_status netlink_batch_add_msg(
        struct nl_batch *bth, struct zebra_dplane_ctx *ctx,
        ssize_t (*msg_encoder)(struct zebra_dplane_ctx *, void *, size_t),
-       bool extra_msg);
+       bool ignore_res);
 
 
 #endif /* HAVE_NETLINK */

diff --git a/zebra/zebra_dplane.c b/zebra/zebra_dplane.c
index 3bff95101c059fd1ee81164c6531dbc3a90dad04..d34e1433ce53d5a099a7fbe44a6b72b9832a0f4a 100644 (file)
--- a/zebra/zebra_dplane.c
+++ b/zebra/zebra_dplane.c
@@ -1977,6 +1977,7 @@ int dplane_ctx_nexthop_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op,
         * it probably won't require two messages
         */
        dplane_ctx_ns_init(ctx, zns, (op == DPLANE_OP_NH_UPDATE));
+       ctx->zd_is_update = (op == DPLANE_OP_NH_UPDATE);
 
        ret = AOK;
 
@@ -1999,6 +2000,7 @@ int dplane_ctx_lsp_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op,
        /* Capture namespace info */
        dplane_ctx_ns_init(ctx, zebra_ns_lookup(NS_DEFAULT),
                           (op == DPLANE_OP_LSP_UPDATE));
+       ctx->zd_is_update = (op == DPLANE_OP_LSP_UPDATE);
 
        memset(&ctx->u.lsp, 0, sizeof(ctx->u.lsp));
 
@@ -2220,6 +2222,7 @@ static int dplane_ctx_rule_init(struct zebra_dplane_ctx *ctx,
 
        dplane_ctx_ns_init(ctx, zebra_ns_lookup(NS_DEFAULT),
                           op == DPLANE_OP_RULE_UPDATE);
+       ctx->zd_is_update = (op == DPLANE_OP_RULE_UPDATE);
 
        ctx->zd_vrf_id = new_rule->vrf_id;
        memcpy(ctx->zd_ifname, new_rule->ifname, sizeof(new_rule->ifname));