On 13 May 2021, at 18:36, Ilya Maximets wrote:
> On 11/24/20 11:43 AM, Eelco Chaudron wrote:
>> Add support for the dec_ttl action. Instead of programming the datapath with
>> a flow that matches the packet TTL and an IP set, use a single dec_ttl
>> action.
>>
>> The old behavior is kept if the new action is not supported by the datapath.
>>
>> # ovs-ofctl dump-flows br0
>> cookie=0x0, duration=12.538s, table=0, n_packets=4, n_bytes=392, ip
>> actions=dec_ttl,NORMAL
>> cookie=0x0, duration=12.536s, table=0, n_packets=4, n_bytes=168,
>> actions=NORMAL
>>
>> # ping -c1 -t 20 192.168.0.2
>> PING 192.168.0.2 (192.168.0.2) 56(84) bytes of data.
>> IP (tos 0x0, ttl 19, id 45336, offset 0, flags [DF], proto ICMP (1),
>> length 84)
>> 192.168.0.1 > 192.168.0.2: ICMP echo request, id 8865, seq 1, length 64
>>
>> Linux netlink datapath support depends on upstream Linux commit:
>> 744676e77720 ("openvswitch: add TTL decrement action")
>>
>>
>> Note that in the Linux kernel tree the OVS_ACTION_ATTR_ADD_MPLS has been
>> defined, and to make sure the IDs are in sync, it had to be added to the
>> OVS source tree. This required some additional case statements, which
>> should be revisited once the OVS implementation is added.
<SNIP>
>> @@ -719,6 +719,55 @@ odp_execute_sample(void *dp, struct dp_packet *packet,
>> bool steal,
>> nl_attr_get_size(subactions), dp_execute_action);
>> }
>>
>> +static bool execute_dec_ttl(struct dp_packet *packet)
>
> Function name should start on a next line.
Ack, will change in the v4.
<SNIP>
>> +static void odp_dec_ttl_exception_handler(void *dp,
>> + struct dp_packet_batch *batch,
>> + const struct nlattr *action,
>> + odp_execute_cb dp_execute_action)
>
> Function name should start on a next line.
Ack, will change in the v4.
<SNIP>
>> +{
>> + static const struct nl_policy dec_ttl_policy[] = {
>> + [OVS_DEC_TTL_ATTR_ACTION] = { .type = NL_A_NESTED },
>> + };
>> + struct nlattr *attrs[ARRAY_SIZE(dec_ttl_policy)];
>> +
>> + if (!nl_parse_nested(action, dec_ttl_policy, attrs, ARRAY_SIZE(attrs))
>> ||
>> + !attrs[OVS_DEC_TTL_ATTR_ACTION]) {
>> + OVS_NOT_REACHED();
>
> This should, probably, be an ovs_assert() instead to have some clue in logs
> if this happened for some reason.
Ack, will change this to ovs_assert() in the v4
<SNIP>
>> +
>> + /* Execute action on packets with ttl <= 1. */
>> + if (invalid_ttl.count > 0) {
>
> dp_packet_batch_size(&invalid_ttl)
Will use !dp_packet_batch_is_empty(&nvalid_ttl)
>> + odp_dec_ttl_exception_handler(dp, &invalid_ttl, a,
>> + dp_execute_action);
>> + }
>> +
>> + if (last_action || !batch->count) {
>
> dp_packet_batch_is_empty(batch)
>
>> + /* We do not need to free the packets. */
>
> Why exactly we do not need to free the packets?
> If for any reason dec_ttl will be the last action in the list we
> will just leak all the packets here. We, probbaly, should not have
> this 'if' condition at all.
Yes, looking at this again, I agree we should not return on last_action. We do
need to return on dp_packet_batch_is_empty(batch) as in this case, all packets
have taken the exception route. I will update this in v4 with the appropriate
comment text.
<SNIP>
>> +/* New handling for dec_ttl action. */
>> +static void
>> +xlate_dec_ttl_action(struct xlate_ctx *ctx, struct ofpact_cnt_ids *ids)
>> +{
>> + struct flow *flow = &ctx->xin->flow;
>> + struct flow_wildcards *wc = ctx->wc;
>> + size_t offset, offset_actions;
>> + size_t i;
>> +
>> + if (!is_ip_any(flow)) {
>> + return;
>> + }
>> +
>> + if (!ctx->xbridge->support.dec_ttl_action
>> + || netdev_is_flow_api_enabled()) {
>> + wc->masks.nw_ttl = 0xff;
>> + compose_dec_ttl(ctx, ids);
>
> compose_dec_ttl() returns boolean value that indicates if translation
> should proceed or not, but the value just ignored here.
> We should return the result from this function and use it in the caller.
Ack, will fix this in V4 in addition to some duplicate work done here and in
compose_dec_ttl().
<SNIP>
>> case OFPACT_DEC_TTL:
>> - wc->masks.nw_ttl = 0xff;
>> - if (compose_dec_ttl(ctx, ofpact_get_DEC_TTL(a))) {
>> - return;
>> - }
>> + xlate_dec_ttl_action(ctx, ofpact_get_DEC_TTL(a));
>
> Here we need to 'return' if xlate_dec_ttl_action() returns true.
Ack, same as above, will fix in v4.
<SNIP>
>
> Since you're ading a new datapath action, tests for tests/odp.at
> are required.
Will add a test in v4.
> On a general note, OVS doesn't use Co-develped-by tag, we're using
> Co-authored-by instead. And co-authors needs to Sign-off.
Thanks for the insight, I just used what was there when I inherited this work
from Matteo. I prepped a v4 and will ask them if I can add their sign-off. Once
done, I’ll send out the v4.
> Best regards, Ilya Maximets.
Thanks for the review!
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
