On Mon, Mar 25, 2019 at 12:07 AM Numan Siddique <nusid...@redhat.com> wrote:
> > > On Fri, Mar 22, 2019 at 9:10 PM Gregory Rose <gvrose8...@gmail.com> wrote: > >> >> On 3/22/2019 2:58 AM, Numan Siddique wrote: >> >> >> >> On Fri, Mar 22, 2019 at 6:16 AM Gregory Rose <gvrose8...@gmail.com> >> wrote: >> >>> >>> >>> On 3/21/2019 5:38 PM, Gregory Rose wrote: >>> > >>> > >>> > On 3/21/2019 10:37 AM, Numan Siddique wrote: >>> >> This is the datapath patch - >>> https://patchwork.ozlabs.org/patch/1046370/ >>> >> and this is the corresponding ovs-vswitchd patch - >>> >> https://patchwork.ozlabs.org/patch/1059081/ (this is part of the >>> >> series - >>> >> https://patchwork.ozlabs.org/project/openvswitch/list/?series=98190, >>> >> but probably you would be interested in only ovs patch) >>> >> >>> >> Sharing the links so that you can find it easily. >>> >> >>> >> Thanks >>> >> Numan >>> >> >>> >> >>> > >>> > This patch: >>> > >>> > https://patchwork.ozlabs.org/patch/1059081/ >>> > >>> > shows this when applied: >>> > >>> > Applying: Add a new OVS action check_pkt_larger >>> > .git/rebase-apply/patch:1097: new blank line at EOF. >>> > + >>> > warning: 1 line adds whitespace errors. >>> > >>> > In regards to the datapath patch 1046370 >>> > <https://patchwork.ozlabs.org/patch/1046370/> >>> > >>> > In execute_check_pkt_len(): >>> > >>> > + >>> > + actual_pkt_len = skb->len + (skb_vlan_tag_present(skb) ? >>> > VLAN_HLEN : 0); >>> > + >>> > >>> > This doesn't seem right to me - the skb length should include the >>> > length of the entire packet, including any >>> > VLAN tags, or at least that is my understanding. Please check it. >>> >> > Hi Greg, > > I checked and tested it it. I can confirm that skb->len doesn't include > the vlan header. > Existing code in flow.c also uses the same way to get the packet len - > https://github.com/openvswitch/ovs/blob/master/datapath/flow.c#L77 > > I submitted the patch to net-dev ML and I forgot to CC - ovs-dev - https://patchwork.ozlabs.org/patch/1063378/ Thanks Numan > Thanks > Numan > > > >>> > In validate_and_copy_check_pkt_len() in flow_netlink.c: >>> > >>> > + static const struct nla_policy pol[OVS_CHECK_PKT_LEN_ATTR_MAX >>> > + 1] = { >>> > + [OVS_CHECK_PKT_LEN_ATTR_PKT_LEN] = {.type = NLA_U16 }, >>> > + [OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER] = { >>> > + .type = NLA_NESTED }, >>> > + [OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL] = { >>> > + .type = NLA_NESTED }, >>> > + }; >>> > >>> > I don't care for declaring these things within function scope and it >>> > is not generally done. I see that >>> > flow_netlink.c has one other instance of the nla_policy structure >>> > statically declared within the function scope >>> > but if you look at datapath.c none of them are. I prefer the way it's >>> > done in datapath.c. I also grepped around >>> > in other kernel code in the ./net tree and that is also the way it's >>> > done there, i.e. I didn't see any other >>> > instances of it declared within function scope. >>> > >>> > I compiled both the ovs-vswitchd and openvswitch kernel module >>> > components with no issues. I wanted to use >>> > clang but the version of clang on Ubuntu right now doesn't have >>> > retpoline support so it won't compile >>> > kernel modules. >>> > >>> > :-/ >>> > >>> > I did some quick regression testing and found no problems. If you can >>> > address the two coding issues I brought up >>> > then I'd be glad to add my reviewed and tested by tags. >>> >>> Oh wait, this is just an RFC. >>> >>> I'll review and test the patches again when they officially come out. >>> Maybe clang will have retpoline support >>> by then. >>> >>> >> Thank you for the review. I will address them. I am planning to submit >> the patch >> to net-next ML. Looks like the window is open now - >> http://vger.kernel.org/~davem/net-next.html >> >> Please let me know in case you prefer to submit another RFC version here >> before submitting to net-dev ML. >> >> >> I think you're good to go. >> >> Thanks, >> >> - Greg >> >> >> Thanks >> Numan >> >> - Greg >>> >>> >> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev