Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 29 Nov 2022 12:00:58 +0100
From:   Horatiu Vultur <horatiu.vultur@microchip.com>
To:     Alexander Lobakin <alexandr.lobakin@intel.com>
CC:     <linux-arm-kernel@lists.infradead.org>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <davem@davemloft.net>,
        <edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>,
        <lars.povlsen@microchip.com>, <Steen.Hegelund@microchip.com>,
        <daniel.machon@microchip.com>, <UNGLinuxDriver@microchip.com>,
        <casper.casan@gmail.com>
Subject: Re: [PATCH net-next] net: microchip: vcap: Change how the rule id is
 generated
Message-ID: <20221129110058.uv2zqjscwyvku46t@soft-dev3-1>
References: <20221128142959.8325-1-horatiu.vultur@microchip.com>
 <20221128144042.2097491-1-alexandr.lobakin@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <20221128144042.2097491-1-alexandr.lobakin@intel.com>
Precedence: bulk

The 11/28/2022 15:40, Alexander Lobakin wrote:

Hi Olek,

> 
> From: Horatiu Vultur <horatiu.vultur@microchip.com>
> Date: Mon, 28 Nov 2022 15:29:59 +0100
> 
> > Currently whenever a new rule id is generated, it picks up the next
> > number bigger than previous id. So it would always be 1, 2, 3, etc.
> > When the rule with id 1 will be deleted and a new rule will be added,
> > it will have the id 4 and not id 1.
> > In theory this can be a problem if at some point a rule will be added
> > and removed ~0 times. Then no more rules can be added because there
> > are no more ids.
> >
> > Change this such that when a new rule is added, search for an empty
> > rule id starting with value of 1 as value 0 is reserved.
> >
> > Fixes: c9da1ac1c212 ("net: microchip: sparx5: Adding initial tc flower support for VCAP API")
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  drivers/net/ethernet/microchip/vcap/vcap_api.c | 7 +------
> >  drivers/net/ethernet/microchip/vcap/vcap_api.h | 1 -
> >  2 files changed, 1 insertion(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c b/drivers/net/ethernet/microchip/vcap/vcap_api.c
> > index b50d002b646dc..b65819f3a927f 100644
> > --- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
> > +++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c
> > @@ -974,17 +974,12 @@ static u32 vcap_next_rule_addr(u32 addr, struct vcap_rule_internal *ri)
> >  /* Assign a unique rule id and autogenerate one if id == 0 */
> >  static u32 vcap_set_rule_id(struct vcap_rule_internal *ri)
> >  {
> > -     u32 next_id;
> > -
> >       if (ri->data.id != 0)
> >               return ri->data.id;
> >
> > -     next_id = ri->vctrl->rule_id + 1;
> > -
> > -     for (next_id = ri->vctrl->rule_id + 1; next_id < ~0; ++next_id) {
> > +     for (u32 next_id = 1; next_id < ~0; ++next_id) {
> >               if (!vcap_lookup_rule(ri->vctrl, next_id)) {
> 
> Or you can simply use IDA/IDR/XArray which takes care of all this :)

Thanks for the great suggestion. From what I can see IDR would be a
great match for this. Then we can also change the function
'vcap_lookup_rule' to use IDR.
On the other side, we will not continuously add/remove entries from the
VCAP, so I was not sure if it is worth bringing the IDR(with the radix
tree) for this.
Anyway I am open for IDR suggestion.
But lets wait for the VCAP maintainer to see what he thinks.

> 
> 
> >                       ri->data.id = next_id;
> > -                     ri->vctrl->rule_id = next_id;
> >                       break;
> >               }
> >       }
> > diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.h b/drivers/net/ethernet/microchip/vcap/vcap_api.h
> > index ca4499838306f..689c7270f2a89 100644
> > --- a/drivers/net/ethernet/microchip/vcap/vcap_api.h
> > +++ b/drivers/net/ethernet/microchip/vcap/vcap_api.h
> > @@ -268,7 +268,6 @@ struct vcap_operations {
> >
> >  /* VCAP API Client control interface */
> >  struct vcap_control {
> > -     u32 rule_id; /* last used rule id (unique across VCAP instances) */
> >       struct vcap_operations *ops;  /* client supplied operations */
> >       const struct vcap_info *vcaps; /* client supplied vcap models */
> >       const struct vcap_statistics *stats; /* client supplied vcap stats */
> > --
> > 2.38.0
> 
> Thanks,
> Olek

-- 
/Horatiu