netdev - Re: [RFC/PATCH net-next v2 4/5] flow_offload: add reoffload process to update hw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20211027144219.GB25471@corigine.com>
Date:   Wed, 27 Oct 2021 16:42:20 +0200
From:   Simon Horman <simon.horman@...igine.com>
To:     Vlad Buslov <vladbu@...dia.com>
Cc:     netdev@...r.kernel.org, Jamal Hadi Salim <jhs@...atatu.com>,
        Roi Dayan <roid@...dia.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...lanox.com>,
        Baowen Zheng <notifications@...hub.com>,
        Louis Peens <louis.peens@...igine.com>,
        oss-drivers@...igine.com, Baowen Zheng <baowen.zheng@...igine.com>
Subject: Re: [RFC/PATCH net-next v2 4/5] flow_offload: add reoffload process
 to update hw_count

Hi Vlad,

On Fri, Oct 01, 2021 at 08:30:38PM +0300, Vlad Buslov wrote:
> 
> On Fri 01 Oct 2021 at 14:32, Simon Horman <simon.horman@...igine.com> wrote:
> > From: Baowen Zheng <baowen.zheng@...igine.com>
> >
> > Add reoffload process to update hw_count when driver
> > is inserted or removed.
> >
> > When reoffloading actions, we still offload the actions
> > that are added independent of filters.

As per comment on 2/4.

	Thanks for your review and sorry for the delay in responding.
	I believe that at this point we have addressed most of the points
	your raised and plan to post a v3 shortly.

	At this point I'd like to relay some responses from Baowen who
	has been working on addressing your review.

> > Change the lock usage to fix sleeping in invalid context.
>
> What does this refer to? Looking at the code it is not clear to me which
> lock usage is changed. Or is it just a change log from v1?

Sorry, this is an artifact of our development process that shouldn't have
been left here. Please ignore.

...

> > @@ -44,6 +45,9 @@ struct tc_action {
> >  	u8			hw_stats;
> >  	u8			used_hw_stats;
> >  	bool			used_hw_stats_valid;
> > +	bool                    add_separate; /* indicate if the action is created
> > +					       * independent of any flow
> > +					       */
> 
> This looks like a duplication of flags since this value is derived from
> BIND flag. I understand that you need this because currently all flags
> that are not visible to the userspace are cleared after action is
> created, but maybe it would be better to refactor the code to preserve
> all flags and to only apply TCA_ACT_FLAGS_USER_MASK when dumping to user
> space.

Thanks, we've cleaned things up as you suggest for v3.

...