* [PATCH RFC, iproute2] tc/mirred: Extend the mirred/redirect action to accept additional traffic class parameter
@ 2017-08-01 0:40 Amritha Nambiar
2017-08-01 3:51 ` Stephen Hemminger
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Amritha Nambiar @ 2017-08-01 0:40 UTC (permalink / raw)
To: stephen, netdev
Cc: alexander.h.duyck, kiran.patil, amritha.nambiar,
sridhar.samudrala, mitch.a.williams, alexander.duyck,
neerav.parikh, carolyn.wyborny, jeffrey.t.kirsher
The Mirred/redirect action is extended to accept a traffic
class on the device in addition to the device's ifindex.
Usage: mirred <DIRECTION> <ACTION> <dev DEVICENAME> <tc TC_INDEX>
Example:
# tc qdisc add dev eth0 ingress
# tc filter add dev eth0 protocol ip parent ffff: prio 1 flower\
dst_ip 192.168.1.1/32 ip_proto udp dst_port 22\
indev eth0 action mirred ingress redirect dev eth0 tc 1
Signed-off-by: Amritha Nambiar <amritha.nambiar@intel.com>
---
tc/m_mirred.c | 26 +++++++++++++++++++++++++-
1 file changed, 25 insertions(+), 1 deletion(-)
diff --git a/include/linux/tc_act/tc_mirred.h b/include/linux/tc_act/tc_mirred.h
index 3d7a2b3..9a3aa61 100644
--- a/include/linux/tc_act/tc_mirred.h
+++ b/include/linux/tc_act/tc_mirred.h
@@ -9,6 +9,9 @@
#define TCA_EGRESS_MIRROR 2 /* mirror packet to EGRESS */
#define TCA_INGRESS_REDIR 3 /* packet redirect to INGRESS*/
#define TCA_INGRESS_MIRROR 4 /* mirror packet to INGRESS */
+
+#define MIRRED_F_TC_MAP 0x1
+#define MIRRED_TC_MAP_MAX 0x10
struct tc_mirred {
tc_gen;
@@ -21,6 +24,7 @@ enum {
TCA_MIRRED_TM,
TCA_MIRRED_PARMS,
TCA_MIRRED_PAD,
+ TCA_MIRRED_TC_MAP,
__TCA_MIRRED_MAX
};
#define TCA_MIRRED_MAX (__TCA_MIRRED_MAX - 1)
diff --git a/tc/m_mirred.c b/tc/m_mirred.c
index 2384bda..1a18c6b 100644
--- a/tc/m_mirred.c
+++ b/tc/m_mirred.c
@@ -29,12 +29,13 @@
static void
explain(void)
{
- fprintf(stderr, "Usage: mirred <DIRECTION> <ACTION> [index INDEX] <dev DEVICENAME>\n");
+ fprintf(stderr, "Usage: mirred <DIRECTION> <ACTION> [index INDEX] <dev DEVICENAME> [tc TCINDEX]\n");
fprintf(stderr, "where:\n");
fprintf(stderr, "\tDIRECTION := <ingress | egress>\n");
fprintf(stderr, "\tACTION := <mirror | redirect>\n");
fprintf(stderr, "\tINDEX is the specific policy instance id\n");
fprintf(stderr, "\tDEVICENAME is the devicename\n");
+ fprintf(stderr, "\tTCINDEX is the traffic class index\n");
}
@@ -72,6 +73,8 @@ parse_direction(struct action_util *a, int *argc_p, char ***argv_p,
struct tc_mirred p = {};
struct rtattr *tail;
char d[16] = {};
+ __u32 flags = 0;
+ __u8 tc;
while (argc > 0) {
@@ -142,6 +145,18 @@ parse_direction(struct action_util *a, int *argc_p, char ***argv_p,
argc--;
argv++;
+ if ((argc > 0) && (matches(*argv, "tc") == 0)) {
+ NEXT_ARG();
+ tc = atoi(*argv);
+ if (tc >= MIRRED_TC_MAP_MAX) {
+ fprintf(stderr, "Invalid TC index\n");
+ return -1;
+ }
+ flags |= MIRRED_F_TC_MAP;
+ ok++;
+ argc--;
+ argv++;
+ }
break;
}
@@ -193,6 +208,9 @@ parse_direction(struct action_util *a, int *argc_p, char ***argv_p,
tail = NLMSG_TAIL(n);
addattr_l(n, MAX_MSG, tca_id, NULL, 0);
addattr_l(n, MAX_MSG, TCA_MIRRED_PARMS, &p, sizeof(p));
+ if (flags & MIRRED_F_TC_MAP)
+ addattr_l(n, MAX_MSG, TCA_MIRRED_TC_MAP,
+ &tc, sizeof(tc));
tail->rta_len = (void *) NLMSG_TAIL(n) - (void *) tail;
*argc_p = argc;
@@ -248,6 +266,7 @@ print_mirred(struct action_util *au, FILE * f, struct rtattr *arg)
struct tc_mirred *p;
struct rtattr *tb[TCA_MIRRED_MAX + 1];
const char *dev;
+ __u8 *tc;
if (arg == NULL)
return -1;
@@ -273,6 +292,11 @@ print_mirred(struct action_util *au, FILE * f, struct rtattr *arg)
fprintf(f, "mirred (%s to device %s)", mirred_n2a(p->eaction), dev);
print_action_control(f, " ", p->action, "");
+ if (tb[TCA_MIRRED_TC_MAP]) {
+ tc = RTA_DATA(tb[TCA_MIRRED_TC_MAP]);
+ fprintf(f, " tc %d", *tc);
+ }
+
fprintf(f, "\n ");
fprintf(f, "\tindex %u ref %d bind %d", p->index, p->refcnt,
p->bindcnt);
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH RFC, iproute2] tc/mirred: Extend the mirred/redirect action to accept additional traffic class parameter
2017-08-01 0:40 [PATCH RFC, iproute2] tc/mirred: Extend the mirred/redirect action to accept additional traffic class parameter Amritha Nambiar
@ 2017-08-01 3:51 ` Stephen Hemminger
2017-08-01 15:12 ` David Laight
2017-08-01 14:02 ` Roman Mashak
2017-08-02 18:41 ` Roopa Prabhu
2 siblings, 1 reply; 7+ messages in thread
From: Stephen Hemminger @ 2017-08-01 3:51 UTC (permalink / raw)
To: Amritha Nambiar
Cc: netdev, alexander.h.duyck, kiran.patil, sridhar.samudrala,
mitch.a.williams, alexander.duyck, neerav.parikh,
carolyn.wyborny, jeffrey.t.kirsher
On Mon, 31 Jul 2017 17:40:50 -0700
Amritha Nambiar <amritha.nambiar@intel.com> wrote:
The concept is fine, bu t the code looks different than the rest which
is never a good sign.
> + if ((argc > 0) && (matches(*argv, "tc") == 0)) {
Extra () are unnecessary in compound conditional.
> + tc = atoi(*argv);
Prefer using strtoul since it has better error handling than atoi()
> + argc--;
> + argv++;
> + }
Use NEXT_ARG() construct like rest of the code.
^ permalink raw reply [flat|nested] 7+ messages in thread* RE: [PATCH RFC, iproute2] tc/mirred: Extend the mirred/redirect action to accept additional traffic class parameter
2017-08-01 3:51 ` Stephen Hemminger
@ 2017-08-01 15:12 ` David Laight
2017-08-02 18:28 ` Nambiar, Amritha
0 siblings, 1 reply; 7+ messages in thread
From: David Laight @ 2017-08-01 15:12 UTC (permalink / raw)
To: 'Stephen Hemminger', Amritha Nambiar
Cc: netdev, alexander.h.duyck, kiran.patil, sridhar.samudrala,
mitch.a.williams, alexander.duyck, neerav.parikh,
carolyn.wyborny, jeffrey.t.kirsher
From: Stephen Hemminger
> Sent: 01 August 2017 04:52
> On Mon, 31 Jul 2017 17:40:50 -0700
> Amritha Nambiar <amritha.nambiar@intel.com> wrote:
> The concept is fine, bu t the code looks different than the rest which
> is never a good sign.
>
>
> > + if ((argc > 0) && (matches(*argv, "tc") == 0)) {
>
> Extra () are unnecessary in compound conditional.
>
> > + tc = atoi(*argv);
>
> Prefer using strtoul since it has better error handling than atoi()
>
> > + argc--;
> > + argv++;
> > + }
>
>
> Use NEXT_ARG() construct like rest of the code.
Why bother faffing about with argc at all?
The argument list terminates when *argv == NULL.
David
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH RFC, iproute2] tc/mirred: Extend the mirred/redirect action to accept additional traffic class parameter
2017-08-01 15:12 ` David Laight
@ 2017-08-02 18:28 ` Nambiar, Amritha
0 siblings, 0 replies; 7+ messages in thread
From: Nambiar, Amritha @ 2017-08-02 18:28 UTC (permalink / raw)
To: David Laight, 'Stephen Hemminger'
Cc: netdev, alexander.h.duyck, kiran.patil, sridhar.samudrala,
mitch.a.williams, alexander.duyck, neerav.parikh,
carolyn.wyborny, jeffrey.t.kirsher
On 8/1/2017 8:12 AM, David Laight wrote:
> From: Stephen Hemminger
>> Sent: 01 August 2017 04:52
>> On Mon, 31 Jul 2017 17:40:50 -0700
>> Amritha Nambiar <amritha.nambiar@intel.com> wrote:
>> The concept is fine, bu t the code looks different than the rest which
>> is never a good sign.
>>
>>
>>> + if ((argc > 0) && (matches(*argv, "tc") == 0)) {
>>
>> Extra () are unnecessary in compound conditional.
>>
>>> + tc = atoi(*argv);
>>
>> Prefer using strtoul since it has better error handling than atoi()
>>
>>> + argc--;
>>> + argv++;
>>> + }
>>
>>
>> Use NEXT_ARG() construct like rest of the code.
>
> Why bother faffing about with argc at all?
> The argument list terminates when *argv == NULL.
>
I'll submit the next version with these fixes.
> David
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RFC, iproute2] tc/mirred: Extend the mirred/redirect action to accept additional traffic class parameter
2017-08-01 0:40 [PATCH RFC, iproute2] tc/mirred: Extend the mirred/redirect action to accept additional traffic class parameter Amritha Nambiar
2017-08-01 3:51 ` Stephen Hemminger
@ 2017-08-01 14:02 ` Roman Mashak
2017-08-02 18:41 ` Roopa Prabhu
2 siblings, 0 replies; 7+ messages in thread
From: Roman Mashak @ 2017-08-01 14:02 UTC (permalink / raw)
To: Amritha Nambiar
Cc: stephen, netdev, alexander.h.duyck, kiran.patil,
sridhar.samudrala, mitch.a.williams, alexander.duyck,
neerav.parikh, carolyn.wyborny, jeffrey.t.kirsher
Amritha Nambiar <amritha.nambiar@intel.com> writes:
[...]
> @@ -72,6 +73,8 @@ parse_direction(struct action_util *a, int *argc_p, char ***argv_p,
> struct tc_mirred p = {};
> struct rtattr *tail;
> char d[16] = {};
> + __u32 flags = 0;
> + __u8 tc;
>
> while (argc > 0) {
>
> @@ -142,6 +145,18 @@ parse_direction(struct action_util *a, int *argc_p, char ***argv_p,
> argc--;
> argv++;
>
> + if ((argc > 0) && (matches(*argv, "tc") == 0)) {
> + NEXT_ARG();
> + tc = atoi(*argv);
Probably better to use strtol() instead, somebody wants to specify hex
base, also it has stronger error checks.
> + if (tc >= MIRRED_TC_MAP_MAX) {
> + fprintf(stderr, "Invalid TC index\n");
> + return -1;
> + }
> + flags |= MIRRED_F_TC_MAP;
> + ok++;
> + argc--;
> + argv++;
> + }
> break;
>
> }
> @@ -193,6 +208,9 @@ parse_direction(struct action_util *a, int *argc_p, char ***argv_p,
> tail = NLMSG_TAIL(n);
> addattr_l(n, MAX_MSG, tca_id, NULL, 0);
> addattr_l(n, MAX_MSG, TCA_MIRRED_PARMS, &p, sizeof(p));
> + if (flags & MIRRED_F_TC_MAP)
> + addattr_l(n, MAX_MSG, TCA_MIRRED_TC_MAP,
> + &tc, sizeof(tc));
> tail->rta_len = (void *) NLMSG_TAIL(n) - (void *) tail;
>
> *argc_p = argc;
> @@ -248,6 +266,7 @@ print_mirred(struct action_util *au, FILE * f, struct rtattr *arg)
> struct tc_mirred *p;
> struct rtattr *tb[TCA_MIRRED_MAX + 1];
> const char *dev;
> + __u8 *tc;
>
> if (arg == NULL)
> return -1;
> @@ -273,6 +292,11 @@ print_mirred(struct action_util *au, FILE * f, struct rtattr *arg)
> fprintf(f, "mirred (%s to device %s)", mirred_n2a(p->eaction), dev);
> print_action_control(f, " ", p->action, "");
>
> + if (tb[TCA_MIRRED_TC_MAP]) {
> + tc = RTA_DATA(tb[TCA_MIRRED_TC_MAP]);
> + fprintf(f, " tc %d", *tc);
'tc' is declared as __u8 so format should be %u
> + }
> +
> fprintf(f, "\n ");
> fprintf(f, "\tindex %u ref %d bind %d", p->index, p->refcnt,
> p->bindcnt);
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH RFC, iproute2] tc/mirred: Extend the mirred/redirect action to accept additional traffic class parameter
2017-08-01 0:40 [PATCH RFC, iproute2] tc/mirred: Extend the mirred/redirect action to accept additional traffic class parameter Amritha Nambiar
2017-08-01 3:51 ` Stephen Hemminger
2017-08-01 14:02 ` Roman Mashak
@ 2017-08-02 18:41 ` Roopa Prabhu
2017-08-02 18:58 ` Nambiar, Amritha
2 siblings, 1 reply; 7+ messages in thread
From: Roopa Prabhu @ 2017-08-02 18:41 UTC (permalink / raw)
To: Amritha Nambiar
Cc: stephen, netdev, Alexander Duyck, kiran.patil, Samudrala,
Sridhar, mitch.a.williams, Alexander Duyck, neerav.parikh,
carolyn.wyborny, Jeff Kirsher
On Mon, Jul 31, 2017 at 5:40 PM, Amritha Nambiar
<amritha.nambiar@intel.com> wrote:
> The Mirred/redirect action is extended to accept a traffic
> class on the device in addition to the device's ifindex.
>
> Usage: mirred <DIRECTION> <ACTION> <dev DEVICENAME> <tc TC_INDEX>
>
> Example:
> # tc qdisc add dev eth0 ingress
>
> # tc filter add dev eth0 protocol ip parent ffff: prio 1 flower\
> dst_ip 192.168.1.1/32 ip_proto udp dst_port 22\
> indev eth0 action mirred ingress redirect dev eth0 tc 1
Can we call the new parameter tclass or something else ?.
with this 'tc' appears twice in the command :)
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RFC, iproute2] tc/mirred: Extend the mirred/redirect action to accept additional traffic class parameter
2017-08-02 18:41 ` Roopa Prabhu
@ 2017-08-02 18:58 ` Nambiar, Amritha
0 siblings, 0 replies; 7+ messages in thread
From: Nambiar, Amritha @ 2017-08-02 18:58 UTC (permalink / raw)
To: Roopa Prabhu
Cc: stephen, netdev, Alexander Duyck, kiran.patil, Samudrala,
Sridhar, mitch.a.williams, Alexander Duyck, neerav.parikh,
carolyn.wyborny, Jeff Kirsher
On 8/2/2017 11:41 AM, Roopa Prabhu wrote:
> On Mon, Jul 31, 2017 at 5:40 PM, Amritha Nambiar
> <amritha.nambiar@intel.com> wrote:
>> The Mirred/redirect action is extended to accept a traffic
>> class on the device in addition to the device's ifindex.
>>
>> Usage: mirred <DIRECTION> <ACTION> <dev DEVICENAME> <tc TC_INDEX>
>>
>> Example:
>> # tc qdisc add dev eth0 ingress
>>
>> # tc filter add dev eth0 protocol ip parent ffff: prio 1 flower\
>> dst_ip 192.168.1.1/32 ip_proto udp dst_port 22\
>> indev eth0 action mirred ingress redirect dev eth0 tc 1
>
>
> Can we call the new parameter tclass or something else ?.
> with this 'tc' appears twice in the command :)
>
Sounds right. I was already thinking of alternatives like 'tcqgroup',
'qgroup' and now we have 'tclass'.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-08-02 18:58 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-01 0:40 [PATCH RFC, iproute2] tc/mirred: Extend the mirred/redirect action to accept additional traffic class parameter Amritha Nambiar
2017-08-01 3:51 ` Stephen Hemminger
2017-08-01 15:12 ` David Laight
2017-08-02 18:28 ` Nambiar, Amritha
2017-08-01 14:02 ` Roman Mashak
2017-08-02 18:41 ` Roopa Prabhu
2017-08-02 18:58 ` Nambiar, Amritha
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.