MEDIUM: filters: use per-channel filter list when relevant

In the historical implementation, all filter related information where
stored at the stream level (using struct strm_flt * context), and filters
iteration was performed at the stream level also.

We identified that this was not ideal and would make the implementation of
future filters more complex since filters ordering should be handled in
a different order during request and response handling for decompression
for instance.

To make such thing possible, in this commit we migrate some channel
specific filter contexts in the channel directly (request or response),
and we implement 2 additional filter lists, one on the request channel
and another on the response channel. The historical stream filter list
is kept as-is because in some contexts only the stream is available and
we have to iterate on all filters. But for functions where we only are
interested in request side or response side filters, we now use dedicated
channel filters list instead.

The only overhead is that the "struct filter" was expanded by two "struct
list".

For now, no change of behavior is expected.

diff --git a/include/haproxy/channel-t.h b/include/haproxy/channel-t.h
index 20afe7d6508887743c6842ea77266b91774aa8e5..a2a11a561bdf98ec752587857415ecffb8dbf25f 100644 (file)
--- a/include/haproxy/channel-t.h
+++ b/include/haproxy/channel-t.h
@@ -24,6 +24,7 @@
 
 #include <haproxy/api-t.h>
 #include <haproxy/buf-t.h>
+#include <haproxy/filters-t.h>
 #include <haproxy/show_flags-t.h>
 
 /* The CF_* macros designate Channel Flags, which may be ORed in the bit field
@@ -205,6 +206,7 @@ struct channel {
        unsigned char xfer_large;       /* number of consecutive large xfers */
        unsigned char xfer_small;       /* number of consecutive small xfers */
        int analyse_exp;                /* expiration date for current analysers (if set) */
+       struct chn_flt flt;             /* current state of filters active on this channel */
 };
 
 

diff --git a/include/haproxy/filters-t.h b/include/haproxy/filters-t.h
index 6c1879fbed9a0b70743a87e4669982e3e220da47..19658d847a08cbe89f70f3f13dca872d3d1edfcc 100644 (file)
--- a/include/haproxy/filters-t.h
+++ b/include/haproxy/filters-t.h
@@ -232,22 +232,28 @@ struct filter {
                                            * 0: request channel, 1: response channel */
        unsigned int    pre_analyzers;     /* bit field indicating analyzers to pre-process */
        unsigned int    post_analyzers;    /* bit field indicating analyzers to post-process */
-       struct list     list;              /* Next filter for the same proxy/stream */
+       struct list     list;              /* Filter list for the stream */
+       /* req_list and res_list are exactly equivalent, except the order may differ */
+       struct list     req_list;          /* Filter list for request channel */
+       struct list     res_list;          /* Filter list for response channel */
 };
 
 /*
  * Structure reprensenting the "global" state of filters attached to a stream.
+ * Doesn't hold much information, as the channel themselves hold chn_flt struct
+ * which contains the per-channel members.
  */
 struct strm_flt {
        struct list    filters;               /* List of filters attached to a stream */
-       struct filter *current[2];            /* From which filter resume processing, for a specific channel.
-                                              * This is used for resumable callbacks only,
-                                              * If NULL, we start from the first filter.
-                                              * 0: request channel, 1: response channel */
        unsigned short flags;                 /* STRM_FL_* */
-       unsigned char  nb_req_data_filters;   /* Number of data filters registered on the request channel */
-       unsigned char  nb_rsp_data_filters;   /* Number of data filters registered on the response channel */
-       unsigned long long offset[2];
+};
+
+/* structure holding filter state for some members that are channel oriented */
+struct chn_flt {
+       struct list filters;                  /* List of filters attached to a channel */
+       struct filter *current;               /* From which filter resume processing, for a specific channel. */
+       unsigned char nb_data_filters;        /* Number of data filters registered on channel */
+       unsigned long long offset;
 };
 
 #endif /* _HAPROXY_FILTERS_T_H */

diff --git a/include/haproxy/filters.h b/include/haproxy/filters.h
index 6a3a7e35b9a872bc9527a850970d5d02facc5ec5..d2c5dd4075a2f9356a5a256c09e38dc0358362af 100644 (file)
--- a/include/haproxy/filters.h
+++ b/include/haproxy/filters.h
@@ -40,13 +40,13 @@ extern const char *fcgi_flt_id;
 /* Useful macros to access per-channel values. It can be safely used inside
  * filters. */
 #define CHN_IDX(chn)     (((chn)->flags & CF_ISRESP) == CF_ISRESP)
-#define FLT_STRM_OFF(s, chn) (strm_flt(s)->offset[CHN_IDX(chn)])
+#define FLT_STRM_OFF(s, chn) (chn->flt.offset)
 #define FLT_OFF(flt, chn) ((flt)->offset[CHN_IDX(chn)])
 
 #define HAS_FILTERS(strm)           ((strm)->strm_flt.flags & STRM_FLT_FL_HAS_FILTERS)
 
-#define HAS_REQ_DATA_FILTERS(strm)  ((strm)->strm_flt.nb_req_data_filters != 0)
-#define HAS_RSP_DATA_FILTERS(strm)  ((strm)->strm_flt.nb_rsp_data_filters != 0)
+#define HAS_REQ_DATA_FILTERS(strm)  ((strm)->req.flt.nb_data_filters != 0)
+#define HAS_RSP_DATA_FILTERS(strm)  ((strm)->res.flt.nb_data_filters != 0)
 #define HAS_DATA_FILTERS(strm, chn) (((chn)->flags & CF_ISRESP) ? HAS_RSP_DATA_FILTERS(strm) : HAS_REQ_DATA_FILTERS(strm))
 
 #define IS_REQ_DATA_FILTER(flt)  ((flt)->flags & FLT_FL_IS_REQ_DATA_FILTER)
@@ -137,14 +137,11 @@ static inline void
 register_data_filter(struct stream *s, struct channel *chn, struct filter *filter)
 {
        if (!IS_DATA_FILTER(filter, chn)) {
-               if (chn->flags & CF_ISRESP) {
+               if (chn->flags & CF_ISRESP)
                        filter->flags |= FLT_FL_IS_RSP_DATA_FILTER;
-                       strm_flt(s)->nb_rsp_data_filters++;
-               }
-               else  {
+               else
                        filter->flags |= FLT_FL_IS_REQ_DATA_FILTER;
-                       strm_flt(s)->nb_req_data_filters++;
-               }
+               chn->flt.nb_data_filters++;
        }
 }
 
@@ -153,15 +150,11 @@ static inline void
 unregister_data_filter(struct stream *s, struct channel *chn, struct filter *filter)
 {
        if (IS_DATA_FILTER(filter, chn)) {
-               if (chn->flags & CF_ISRESP) {
+               if (chn->flags & CF_ISRESP)
                        filter->flags &= ~FLT_FL_IS_RSP_DATA_FILTER;
-                       strm_flt(s)->nb_rsp_data_filters--;
-
-               }
-               else  {
+               else
                        filter->flags &= ~FLT_FL_IS_REQ_DATA_FILTER;
-                       strm_flt(s)->nb_req_data_filters--;
-               }
+               chn->flt.nb_data_filters--;
        }
 }
 
@@ -186,9 +179,16 @@ static inline struct filter *flt_list_start(struct stream *strm, struct channel
 {
        struct filter *filter;
 
-       filter = LIST_NEXT(&strm_flt(strm)->filters, struct filter *, list);
-       if (&filter->list == &strm_flt(strm)->filters)
-               filter = NULL; /* empty list */
+       if (chn->flags & CF_ISRESP) {
+               filter = LIST_NEXT(&chn->flt.filters, struct filter *, res_list);
+               if (&filter->res_list == &chn->flt.filters)
+                       filter = NULL; /* empty list */
+       }
+       else {
+               filter = LIST_NEXT(&chn->flt.filters, struct filter *, req_list);
+               if (&filter->req_list == &chn->flt.filters)
+                       filter = NULL; /* empty list */
+       }
 
        return filter;
 }
@@ -196,9 +196,16 @@ static inline struct filter *flt_list_start(struct stream *strm, struct channel
 static inline struct filter *flt_list_next(struct stream *strm, struct channel *chn,
                                            struct filter *filter)
 {
-       filter = LIST_NEXT(&filter->list, struct filter *, list);
-       if (&filter->list == &strm_flt(strm)->filters)
-               filter = NULL; /* end of list */
+       if (chn->flags & CF_ISRESP) {
+               filter = LIST_NEXT(&filter->res_list, struct filter *, res_list);
+               if (&filter->res_list == &chn->flt.filters)
+                       filter = NULL; /* end of list */
+       }
+       else {
+               filter = LIST_NEXT(&filter->req_list, struct filter *, req_list);
+               if (&filter->req_list == &chn->flt.filters)
+                       filter = NULL; /* end of list */
+       }
 
        return filter;
 }

diff --git a/src/filters.c b/src/filters.c
index 0596d9a383398e0d7a8018b3cf7396c319b94df1..f12f16e2e2821e76fc5071d52d80e54c452828f7 100644 (file)
--- a/src/filters.c
+++ b/src/filters.c
@@ -67,9 +67,9 @@ static inline struct filter *resume_filter_list_start(struct stream *strm, struc
 {
        struct filter *filter;
 
-       if (strm_flt(strm)->current[CHN_IDX(chn)]) {
-               filter = strm_flt(strm)->current[CHN_IDX(chn)];
-               strm_flt(strm)->current[CHN_IDX(chn)] = NULL;
+       if (chn->flt.current) {
+               filter = chn->flt.current;
+               chn->flt.current = NULL;
                if (!(chn_prod(chn)->flags & SC_FL_ERROR) &&
                    !(chn->flags & (CF_READ_TIMEOUT|CF_WRITE_TIMEOUT))) {
                        (strm)->waiting_entity.type = STRM_ENTITY_NONE;
@@ -100,7 +100,7 @@ static inline void resume_filter_list_break(struct stream *strm, struct channel
                strm->last_entity.type = STRM_ENTITY_FILTER;
                strm->last_entity.ptr = filter;
        }
-       strm_flt(strm)->current[CHN_IDX(chn)] = filter;
+       chn->flt.current = filter;
 }
 
 /* List head of all known filter keywords */
@@ -455,6 +455,14 @@ flt_stream_add_filter(struct stream *s, struct flt_conf *fconf, unsigned int fla
        }
 
        LIST_APPEND(&strm_flt(s)->filters, &f->list);
+
+       /* for now f->req_list == f->res_list to preserve
+        * historical behavior, but the ordering will change
+        * in the future
+        */
+       LIST_APPEND(&s->req.flt.filters, &f->req_list);
+       LIST_APPEND(&s->res.flt.filters, &f->res_list);
+
        strm_flt(s)->flags |= STRM_FLT_FL_HAS_FILTERS;
        return 0;
 }
@@ -470,6 +478,10 @@ flt_stream_init(struct stream *s)
 
        memset(strm_flt(s), 0, sizeof(*strm_flt(s)));
        LIST_INIT(&strm_flt(s)->filters);
+       memset(&s->req.flt, 0, sizeof(s->req.flt));
+       LIST_INIT(&s->req.flt.filters);
+       memset(&s->res.flt, 0, sizeof(s->res.flt));
+       LIST_INIT(&s->res.flt.filters);
        list_for_each_entry(fconf, &strm_fe(s)->filter_configs, list) {
                if (flt_stream_add_filter(s, fconf, 0) < 0)
                        return -1;
@@ -494,6 +506,8 @@ flt_stream_release(struct stream *s, int only_backend)
                        if (FLT_OPS(filter)->detach)
                                FLT_OPS(filter)->detach(s, filter);
                        LIST_DELETE(&filter->list);
+                       LIST_DELETE(&filter->req_list);
+                       LIST_DELETE(&filter->res_list);
                        pool_free(pool_head_filter, filter);
                }
        }

diff --git a/src/stream.c b/src/stream.c
index f7709072901edfc31579f2974e94fc9bb801f6ac..8913773010e345ef539d836fc71bd02f0d859356 100644 (file)
--- a/src/stream.c
+++ b/src/stream.c
@@ -3771,8 +3771,8 @@ static void __strm_dump_to_buffer(struct buffer *buf, const struct show_sess_ctx
                              htx, htx->flags, htx->size, htx->data, htx_nbblks(htx),
                              (htx->tail >= htx->head) ? "NO" : "YES");
        }
-       if (HAS_FILTERS(strm) && strm->strm_flt.current[0]) {
-               const struct filter *flt = strm->strm_flt.current[0];
+       if (HAS_FILTERS(strm) && strm->req.flt.current) {
+               const struct filter *flt = strm->req.flt.current;
 
                chunk_appendf(buf, "%s      current_filter=%p (id=\"%s\" flags=0x%x pre=0x%x post=0x%x) \n", pfx,
                              flt, flt->config->id, flt->flags, flt->pre_analyzers, flt->post_analyzers);
@@ -3804,8 +3804,8 @@ static void __strm_dump_to_buffer(struct buffer *buf, const struct show_sess_ctx
                              (htx->tail >= htx->head) ? "NO" : "YES");
        }
 
-       if (HAS_FILTERS(strm) && strm->strm_flt.current[1]) {
-               const struct filter *flt = strm->strm_flt.current[1];
+       if (HAS_FILTERS(strm) && strm->res.flt.current) {
+               const struct filter *flt = strm->res.flt.current;
 
                chunk_appendf(buf, "%s      current_filter=%p (id=\"%s\" flags=0x%x pre=0x%x post=0x%x) \n", pfx,
                              flt, flt->config->id, flt->flags, flt->pre_analyzers, flt->post_analyzers);