Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

[Jamal Hadi Salim <jhs@mojatatu.com>]

[-- Attachment #1: Type: text/plain, Size: 617 bytes --]

On 16-08-29 06:38 AM, Jamal Hadi Salim wrote:
> Let me see if i can
> try something. Trickery with RCU is not one of my virtues -
> so i may get it wrong but hope you can comment.

Something like attached? Compile tested.
I tried to emulate gact - but greping around I havent
seen sample code which uses  rcu_read_un/lock + synchronize_rcu
this way
[so be gentle if i got it wrong ;->]

I am not sure where Cong is going but probably introducing
a rcu head in each policy will allow to swap and free old one
with call_rcu may be a better approach. The cost may come in
slowing down replace operations.

cheers,
jamal

[-- Attachment #2: skbmod-rcu-maybe --]
[-- Type: text/plain, Size: 2894 bytes --]


static int tcf_skbmod_run(struct sk_buff *skb, const struct tc_action *a,
			  struct tcf_result *res)
{
	struct tcf_skbmod *d = to_skbmod(a);
	int action = READ_ONCE(d->tcf_action);
	u64 flags = READ_ONCE(d->flags);

	tcf_lastuse_update(&d->tcf_tm);
	bstats_cpu_update(this_cpu_ptr(d->common.cpu_bstats), skb);
	if (unlikely(action == TC_ACT_SHOT)) {
		qstats_drop_inc(this_cpu_ptr(d->common.cpu_qstats));
		return action;
	}

	flags = READ_ONCE(d->flags);
	rcu_read_lock();
	if (flags & SKBMOD_F_DMAC)
		ether_addr_copy(eth_hdr(skb)->h_dest, d->eth_dst);
	if (flags & SKBMOD_F_SMAC)
		ether_addr_copy(eth_hdr(skb)->h_source, d->eth_src);
	if (flags & SKBMOD_F_ETYPE)
		eth_hdr(skb)->h_proto = d->eth_type;
	if (flags & SKBMOD_F_SWAPMAC) {
		u8 tmpaddr[ETH_ALEN];
		/*XXX: I am sure we can come up with something more efficient */
		ether_addr_copy(tmpaddr, eth_hdr(skb)->h_dest);
		ether_addr_copy(eth_hdr(skb)->h_dest, eth_hdr(skb)->h_source);
		ether_addr_copy(eth_hdr(skb)->h_source, tmpaddr);
	}
	rcu_read_unlock();

	return action;
}

static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
			   struct nlattr *est, struct tc_action **a,
			   int ovr, int bind)
{
	struct tc_action_net *tn = net_generic(net, skbmod_net_id);
	struct nlattr *tb[TCA_SKBMOD_MAX + 1];
	struct tc_skbmod *parm;
	struct tcf_skbmod *d;
	u32 lflags = 0;
	u8 *daddr = NULL;
	u8 *saddr = NULL;
	u16 eth_type = 0;

	bool exists = false;
	int ret = 0, err;

	if (nla == NULL)
		return -EINVAL;

	err = nla_parse_nested(tb, TCA_SKBMOD_MAX, nla, skbmod_policy);
	if (err < 0)
		return err;

	if (tb[TCA_SKBMOD_PARMS] == NULL)
		return -EINVAL;

	if (tb[TCA_SKBMOD_DMAC]) {
		daddr = nla_data(tb[TCA_SKBMOD_DMAC]);
		lflags |= SKBMOD_F_DMAC;
	}

	if (tb[TCA_SKBMOD_SMAC]) {
		saddr = nla_data(tb[TCA_SKBMOD_SMAC]);
		lflags |= SKBMOD_F_SMAC;
	}

	if (tb[TCA_SKBMOD_ETYPE]) {
		eth_type = nla_get_u16(tb[TCA_SKBMOD_ETYPE]);
		lflags |= SKBMOD_F_ETYPE;
	}

	parm = nla_data(tb[TCA_SKBMOD_PARMS]);

	if (parm->flags & SKBMOD_F_SWAPMAC)
		lflags = SKBMOD_F_SWAPMAC;

	exists = tcf_hash_check(tn, parm->index, a, bind);
	if (exists && bind)
		return 0;

	if (!lflags) {
		return -EINVAL;
	}

	if (!exists) {
		ret = tcf_hash_create(tn, parm->index, est, a,
				      &act_skbmod_ops, bind, false);
		if (ret)
			return ret;

		d = to_skbmod(*a);
		ret = ACT_P_CREATED;
	} else {
		d = to_skbmod(*a);
		tcf_hash_release(*a, bind);
		if (!ovr)
			return -EEXIST;
	}

	ASSERT_RTNL();
	d->flags = lflags;
	d->tcf_action = parm->action;

	if (ovr)
		spin_lock_bh(&d->tcf_lock);

	if (lflags & SKBMOD_F_DMAC)
		ether_addr_copy(d->eth_dst, daddr);
	if (lflags & SKBMOD_F_SMAC)
		ether_addr_copy(d->eth_src, saddr);
	if (lflags & SKBMOD_F_ETYPE)
		d->eth_type = htons(eth_type);


	if (ovr) {
		spin_unlock_bh(&d->tcf_lock);
		synchronize_rcu();
	}

	if (ret == ACT_P_CREATED)
		tcf_hash_insert(tn, *a);
	return ret;
}