On Thu, Oct 19, 2017 at 05:19:55AM +0800, Jiri Benc wrote:
> On Mon, 16 Oct 2017 21:53:29 +0800, Yi Yang wrote:
> > +static int set_nsh(struct sk_buff *skb, struct sw_flow_key *flow_key,
> > + const struct nlattr *a)
> > +{
> > + struct nshhdr *nh;
> > + size_t length;
> > + int err;
> > + u8 flags;
> > + u8 ttl;
> > + int i;
> > +
> > + struct ovs_key_nsh key;
> > + struct ovs_key_nsh mask;
> > +
> > + err = nsh_key_from_nlattr(a, &key, &mask);
> > + if (err)
> > + return err;
> > +
> > + /* Make sure the NSH base header is there */
> > + err = skb_ensure_writable(skb, skb_network_offset(skb) +
> > + NSH_BASE_HDR_LEN);
> > + if (unlikely(err))
> > + return err;
> > +
> > + nh = nsh_hdr(skb);
> > + length = nsh_hdr_len(nh);
> > +
> > + /* Make sure the whole NSH header is there */
> > + err = skb_ensure_writable(skb, skb_network_offset(skb) +
> > + length);
>
> Calling skb_ensure_writable twice is an unnecessary waste in the fast
> path. If anything, the first call should be changed to pskb_may_pull.
>
> But what we really should do here is to move the header only once. We
> know how much data we're going to write, we have everything stored in
> the key and can calculate it from there.
Actually mdtype can't be set, only push_nsh can set mdtype, so set_nsh
won't have mdtype flow key, we can't get it from flow_key in set_nsh,
only ttl, flags and path_hdr can be set in set_nsh as you can see in code.
I understand your concern is calling skb_ensure_writable twice, so
changing the first one to pskb_may_pull is more appropriate for set_nsh.
>
> length = NSH_BASE_HDR_LEN;
> switch (flow_key->nsh.base.mdtype) {
> case NSH_MD_TYPE1:
> length += sizeof(struct ovs_nsh_key_md1);
> break;
> case NSH_MD_TYPE2:
> length += whatever_way_we_store_the_tlvs_in_flow_key;
> break;
> }
> err = skb_ensure_writable(skb, skb_network_offset(skb) + length);
>
> However, set_nsh does not support MD type 2, thus the second case is a
> dead code. In both switches in this function. As such, it should be
> removed and added only when MD type 2 is introduced. I'd still keep the
> overall logic to ease the future addition, though. This boils down to:
>
> length = NSH_BASE_HDR_LEN;
> /* Assume MD type 1. This function cannot be called for anything
> * else currently. When MD type 2 is added, the line below will
> * have to be turned into a switch on flow_key->nsh.base.mdtype.
> */
> length += sizeof(struct ovs_nsh_key_md1);
> err = skb_ensure_writable(skb, skb_network_offset(skb) + length);
> ...
> flow_key->nsh.base.path_hdr = nh->path_hdr;
> /* Only MD type 1, see the comment above. */
> for (i = 0; i < NSH_MD1_CONTEXT_SIZE; i++) {
> ...
>
> Please verify I'm not missing something.
>
> It seems we also rely on the user space checking first whether the
> packet is indeed compatible with the pushed key/mask. Most importantly,
> that it's of the same mdtype and (in the future) that the MD type 2
> TLVs being written actually fit. Seems this is done the same way in the
> other set_* actions, thus fine with me.
>
> Jiri
