Re: [PATCH net-next 2/3] net/act_pedit: Support using offset relative to the conventional network headers
On Fri, Dec 2, 2016 at 12:40 PM, Amir Vadai wrote:
> On Thu, Dec 01, 2016 at 02:41:14PM -0500, David Miller wrote:
>> From: Amir Vadai
>> Date: Wed, 30 Nov 2016 11:09:27 +0200
>> > +static int pedit_skb_hdr_offset(struct sk_buff *skb,
>> > + enum pedit_header_type htype, int *hoffset)
>> > +{
>> > + int ret = -1;
>> > +
>> > + switch (htype) {
>> > + case PEDIT_HDR_TYPE_ETH:
>> > + if (skb_mac_header_was_set(skb)) {
>> > + *hoffset = skb_mac_offset(skb);
>> > + ret = 0;
>> > + }
>> > + break;
>> > + case PEDIT_HDR_TYPE_RAW:
>> > + case PEDIT_HDR_TYPE_IP4:
>> > + case PEDIT_HDR_TYPE_IP6:
>> > + *hoffset = skb_network_offset(skb);
>> > + ret = 0;
>> > + break;
>> > + case PEDIT_HDR_TYPE_TCP:
>> > + case PEDIT_HDR_TYPE_UDP:
>> > + if (skb_transport_header_was_set(skb)) {
>> > + *hoffset = skb_transport_offset(skb);
>> > + ret = 0;
>> > + }
>> > + break;
>> > + };
>> > +
>> > + return ret;
>> > +}
>> > +
>> The only distinction between the cases is "L2", "L3", and "L4".
>> Therefore I don't see any reason to break it down into IP4 vs. IP6 vs.
>> RAW, for example. They all map to the same thing.
>> So why not just have PEDIT_HDR_TYPE_L2, PEDIT_HDR_TYPE_L3, and
>> PEDIT_HDR_TYPE_L4? It definitely seems more straightforward
>> and cleaner that way.
> Yeh, is isn't by mistake. The next step will be to implement hardware
> offloading of the action, and for that we would like to keep the
> information about the specific header type.
Hi Dave,
I see that this patch is marked as "Changes Requested" @ your patchworks.
Just wanted to make a note as Amir explained here and as mentioned on
the change log, this was done in purpose, as heads up for HW offloads.
Typically HW APIs would let you do things also based on header type
they have parsed, etc, so that's why we added this small redundancy
e.g of IPv4/IPv6 header ID instead of network header ID - while SW
wise both IPv4/IPv6 are using the same code path, for HW offloads, the
HW driver could choose to use the IPv4/IPv6 header ID info.
Or.
Re: [PATCH net-next 2/3] net/act_pedit: Support using offset relative to the conventional network headers
On Thu, Dec 01, 2016 at 02:41:14PM -0500, David Miller wrote:
> From: Amir Vadai
> Date: Wed, 30 Nov 2016 11:09:27 +0200
>
> > @@ -119,18 +119,45 @@ static bool offset_valid(struct sk_buff *skb, int
> > offset)
> > return true;
> > }
> >
> > +static int pedit_skb_hdr_offset(struct sk_buff *skb,
> > + enum pedit_header_type htype, int *hoffset)
> > +{
> > + int ret = -1;
> > +
> > + switch (htype) {
> > + case PEDIT_HDR_TYPE_ETH:
> > + if (skb_mac_header_was_set(skb)) {
> > + *hoffset = skb_mac_offset(skb);
> > + ret = 0;
> > + }
> > + break;
> > + case PEDIT_HDR_TYPE_RAW:
> > + case PEDIT_HDR_TYPE_IP4:
> > + case PEDIT_HDR_TYPE_IP6:
> > + *hoffset = skb_network_offset(skb);
> > + ret = 0;
> > + break;
> > + case PEDIT_HDR_TYPE_TCP:
> > + case PEDIT_HDR_TYPE_UDP:
> > + if (skb_transport_header_was_set(skb)) {
> > + *hoffset = skb_transport_offset(skb);
> > + ret = 0;
> > + }
> > + break;
> > + };
> > +
> > + return ret;
> > +}
> > +
>
> The only distinction between the cases is "L2", "L3", and "L4".
>
> Therefore I don't see any reason to break it down into IP4 vs. IP6 vs.
> RAW, for example. They all map to the same thing.
>
> So why not just have PEDIT_HDR_TYPE_L2, PEDIT_HDR_TYPE_L3, and
> PEDIT_HDR_TYPE_L4? It definitely seems more straightforward
> and cleaner that way.
Yeh, is isn't by mistake. The next step will be to implement hardware
offloading of the action, and for that we would like to keep the
information about the specific header type.
>
> Thanks.
Re: [PATCH net-next 2/3] net/act_pedit: Support using offset relative to the conventional network headers
From: Amir Vadai
Date: Wed, 30 Nov 2016 11:09:27 +0200
> @@ -119,18 +119,45 @@ static bool offset_valid(struct sk_buff *skb, int
> offset)
> return true;
> }
>
> +static int pedit_skb_hdr_offset(struct sk_buff *skb,
> + enum pedit_header_type htype, int *hoffset)
> +{
> + int ret = -1;
> +
> + switch (htype) {
> + case PEDIT_HDR_TYPE_ETH:
> + if (skb_mac_header_was_set(skb)) {
> + *hoffset = skb_mac_offset(skb);
> + ret = 0;
> + }
> + break;
> + case PEDIT_HDR_TYPE_RAW:
> + case PEDIT_HDR_TYPE_IP4:
> + case PEDIT_HDR_TYPE_IP6:
> + *hoffset = skb_network_offset(skb);
> + ret = 0;
> + break;
> + case PEDIT_HDR_TYPE_TCP:
> + case PEDIT_HDR_TYPE_UDP:
> + if (skb_transport_header_was_set(skb)) {
> + *hoffset = skb_transport_offset(skb);
> + ret = 0;
> + }
> + break;
> + };
> +
> + return ret;
> +}
> +
The only distinction between the cases is "L2", "L3", and "L4".
Therefore I don't see any reason to break it down into IP4 vs. IP6 vs.
RAW, for example. They all map to the same thing.
So why not just have PEDIT_HDR_TYPE_L2, PEDIT_HDR_TYPE_L3, and
PEDIT_HDR_TYPE_L4? It definitely seems more straightforward
and cleaner that way.
Thanks.
