| Message ID | 20200710120718.38633-2-sriharsha.basavapatna@broadcom.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | netdev datapath offload: misc fixes | expand |
On 7/10/2020 3:07 PM, Sriharsha Basavapatna wrote: > The offload layer doesn't initialize the 'transfer' attribute > for mark/rss offload (partial offload). It should be set to 0. > > Fixes: 60e778c7533a ("netdev-offload-dpdk: Framework for actions offload.") It is not a bug. .ingress = 1 is also sufficient. See http://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html#Initializing-Structure-Members Anyway, this is the commit that introduced it. e8a2b5bf92bb ("netdev-dpdk: implement flow offload with rte flow") > Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com> > --- > lib/netdev-offload-dpdk.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c > index 26a75f0f2..4c652fd82 100644 > --- a/lib/netdev-offload-dpdk.c > +++ b/lib/netdev-offload-dpdk.c > @@ -818,7 +818,8 @@ netdev_offload_dpdk_mark_rss(struct flow_patterns *patterns, > .group = 0, > .priority = 0, > .ingress = 1, > - .egress = 0 > + .egress = 0, > + .transfer = 0 > }; > struct rte_flow_error error; > struct rte_flow *flow;
On Mon, Jul 13, 2020 at 1:07 PM Eli Britstein <elibr@mellanox.com> wrote: > > > On 7/10/2020 3:07 PM, Sriharsha Basavapatna wrote: > > The offload layer doesn't initialize the 'transfer' attribute > > for mark/rss offload (partial offload). It should be set to 0. > > > > Fixes: 60e778c7533a ("netdev-offload-dpdk: Framework for actions offload.") > > It is not a bug. .ingress = 1 is also sufficient. ingress and transfer are different attributes. > > See > http://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html#Initializing-Structure-Members Let us set it explicitly to 0, like other members (group etc). > > Anyway, this is the commit that introduced it. > > e8a2b5bf92bb ("netdev-dpdk: implement flow offload with rte flow") Ok, thanks. -Harsha > > > Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com> > > --- > > lib/netdev-offload-dpdk.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c > > index 26a75f0f2..4c652fd82 100644 > > --- a/lib/netdev-offload-dpdk.c > > +++ b/lib/netdev-offload-dpdk.c > > @@ -818,7 +818,8 @@ netdev_offload_dpdk_mark_rss(struct flow_patterns *patterns, > > .group = 0, > > .priority = 0, > > .ingress = 1, > > - .egress = 0 > > + .egress = 0, > > + .transfer = 0 > > }; > > struct rte_flow_error error; > > struct rte_flow *flow;
On 7/13/2020 10:55 AM, Sriharsha Basavapatna wrote: > On Mon, Jul 13, 2020 at 1:07 PM Eli Britstein <elibr@mellanox.com> wrote: >> >> On 7/10/2020 3:07 PM, Sriharsha Basavapatna wrote: >>> The offload layer doesn't initialize the 'transfer' attribute >>> for mark/rss offload (partial offload). It should be set to 0. >>> >>> Fixes: 60e778c7533a ("netdev-offload-dpdk: Framework for actions offload.") >> It is not a bug. .ingress = 1 is also sufficient. > ingress and transfer are different attributes. Need to initialize only the non-zero fields. Others are implicitly initialized to zero. > >> See >> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.gnu.org%2Fsoftware%2Fgnu-c-manual%2Fgnu-c-manual.html%23Initializing-Structure-Members&data=02%7C01%7Celibr%40mellanox.com%7Cadfaff15fd8e4cec042808d827021b34%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637302237280357657&sdata=KCkgeMHqrVUtVkPvciFB4cLO%2FiN%2BTP9WZmcaXMf2LqY%3D&reserved=0 > Let us set it explicitly to 0, like other members (group etc). >> Anyway, this is the commit that introduced it. >> >> e8a2b5bf92bb ("netdev-dpdk: implement flow offload with rte flow") Remove the fixes. It doesn't fix anything. You can explain to make it explicit in the commit msg. > Ok, thanks. > -Harsha > >>> Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com> >>> --- >>> lib/netdev-offload-dpdk.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >>> index 26a75f0f2..4c652fd82 100644 >>> --- a/lib/netdev-offload-dpdk.c >>> +++ b/lib/netdev-offload-dpdk.c >>> @@ -818,7 +818,8 @@ netdev_offload_dpdk_mark_rss(struct flow_patterns *patterns, >>> .group = 0, >>> .priority = 0, >>> .ingress = 1, >>> - .egress = 0 >>> + .egress = 0, >>> + .transfer = 0 >>> }; >>> struct rte_flow_error error; >>> struct rte_flow *flow;
On Mon, Jul 13, 2020 at 1:28 PM Eli Britstein <elibr@mellanox.com> wrote: > > > On 7/13/2020 10:55 AM, Sriharsha Basavapatna wrote: > > On Mon, Jul 13, 2020 at 1:07 PM Eli Britstein <elibr@mellanox.com> wrote: > >> > >> On 7/10/2020 3:07 PM, Sriharsha Basavapatna wrote: > >>> The offload layer doesn't initialize the 'transfer' attribute > >>> for mark/rss offload (partial offload). It should be set to 0. > >>> > >>> Fixes: 60e778c7533a ("netdev-offload-dpdk: Framework for actions offload.") > >> It is not a bug. .ingress = 1 is also sufficient. > > ingress and transfer are different attributes. > Need to initialize only the non-zero fields. Others are implicitly > initialized to zero. > > > >> See > >> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.gnu.org%2Fsoftware%2Fgnu-c-manual%2Fgnu-c-manual.html%23Initializing-Structure-Members&data=02%7C01%7Celibr%40mellanox.com%7Cadfaff15fd8e4cec042808d827021b34%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637302237280357657&sdata=KCkgeMHqrVUtVkPvciFB4cLO%2FiN%2BTP9WZmcaXMf2LqY%3D&reserved=0 > > Let us set it explicitly to 0, like other members (group etc). > >> Anyway, this is the commit that introduced it. > >> > >> e8a2b5bf92bb ("netdev-dpdk: implement flow offload with rte flow") > Remove the fixes. It doesn't fix anything. You can explain to make it > explicit in the commit msg. I thought about it, the concern I had was uninitialized value for the transfer field. Like you pointed out, since it is not an issue (even for local struct members), I'll drop this patch. I updated the second patch (L4 proto id) with the right 'fixes' as per your previous comment. Did you look at the 3rd patch in this set ? any comments ? Thanks, -Harsha > > Ok, thanks. > > -Harsha > > > >>> Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com> > >>> --- > >>> lib/netdev-offload-dpdk.c | 3 ++- > >>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c > >>> index 26a75f0f2..4c652fd82 100644 > >>> --- a/lib/netdev-offload-dpdk.c > >>> +++ b/lib/netdev-offload-dpdk.c > >>> @@ -818,7 +818,8 @@ netdev_offload_dpdk_mark_rss(struct flow_patterns *patterns, > >>> .group = 0, > >>> .priority = 0, > >>> .ingress = 1, > >>> - .egress = 0 > >>> + .egress = 0, > >>> + .transfer = 0 > >>> }; > >>> struct rte_flow_error error; > >>> struct rte_flow *flow;
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c index 26a75f0f2..4c652fd82 100644 --- a/lib/netdev-offload-dpdk.c +++ b/lib/netdev-offload-dpdk.c @@ -818,7 +818,8 @@ netdev_offload_dpdk_mark_rss(struct flow_patterns *patterns, .group = 0, .priority = 0, .ingress = 1, - .egress = 0 + .egress = 0, + .transfer = 0 }; struct rte_flow_error error; struct rte_flow *flow;
The offload layer doesn't initialize the 'transfer' attribute for mark/rss offload (partial offload). It should be set to 0. Fixes: 60e778c7533a ("netdev-offload-dpdk: Framework for actions offload.") Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com> --- lib/netdev-offload-dpdk.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)