From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <konstantin.ananyev@intel.com>
Received: from mga03.intel.com (mga03.intel.com [134.134.136.65])
 by dpdk.org (Postfix) with ESMTP id A500C7CE2
 for <dev@dpdk.org>; Mon, 18 Sep 2017 12:57:06 +0200 (CEST)
Received: from orsmga002.jf.intel.com ([10.7.209.21])
 by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;
 18 Sep 2017 03:57:05 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.42,412,1500966000"; d="scan'208";a="136528488"
Received: from irsmsx108.ger.corp.intel.com ([163.33.3.3])
 by orsmga002.jf.intel.com with ESMTP; 18 Sep 2017 03:57:04 -0700
Received: from irsmsx105.ger.corp.intel.com ([169.254.7.75]) by
 IRSMSX108.ger.corp.intel.com ([169.254.11.167]) with mapi id 14.03.0319.002;
 Mon, 18 Sep 2017 11:57:03 +0100
From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: "Richardson, Bruce" <bruce.richardson@intel.com>, Thomas Monjalon
 <thomas@monjalon.net>
CC: "stephen@networkplumber.org" <stephen@networkplumber.org>, "dev@dpdk.org"
 <dev@dpdk.org>, Shahaf Shuler <shahafs@mellanox.com>
Thread-Topic: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the new
 offloads API
Thread-Index: AQHTJYU8DF/tpFi9okyg3kyCdEiysqKkwPgQgAEZ2gCAABLvoIAAIFYAgAATnACAAS25gIAAR4XggAryaYCAAB5GAIAAF+WAgAAUKND///a4gIAAilcAgACvEQCABnMXgIAAFgTA
Date: Mon, 18 Sep 2017 10:57:03 +0000
Message-ID: <2601191342CEEE43887BDE71AB9772584F24BFBF@irsmsx105.ger.corp.intel.com>
References: <cover.1504508374.git.shahafs@mellanox.com>
 <12544923.ZPp1eIik3W@xps>
 <2601191342CEEE43887BDE71AB9772584F24AA9D@irsmsx105.ger.corp.intel.com>
 <6120098.SpXJOblVKo@xps>
 <20170918103154.GA13172@bricha3-MOBL3.ger.corp.intel.com>
In-Reply-To: <20170918103154.GA13172@bricha3-MOBL3.ger.corp.intel.com>
Accept-Language: en-IE, en-US
Content-Language: en-US
X-MS-Has-Attach: 
X-MS-TNEF-Correlator: 
x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZTg5ZGEzMDUtZWM0NC00NDlhLWJiYTctY2JhNWQyMmYwYjdiIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6IjhGaXBDbG9VbStXR3BOUThFd3g0RHBUWUZlbWRXalRublZ3bXRNMDlqSjQ9In0=
x-ctpclassification: CTP_IC
dlp-product: dlpe-windows
dlp-version: 11.0.0.116
dlp-reaction: no-action
x-originating-ip: [163.33.239.180]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the new
 offloads API
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Mon, 18 Sep 2017 10:57:07 -0000



> -----Original Message-----
> From: Richardson, Bruce
> Sent: Monday, September 18, 2017 11:32 AM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; stephen@networkpl=
umber.org; dev@dpdk.org; Shahaf Shuler
> <shahafs@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the ne=
w offloads API
>=20
> On Thu, Sep 14, 2017 at 10:02:26AM +0200, Thomas Monjalon wrote:
> > 13/09/2017 23:42, Ananyev, Konstantin:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > 13/09/2017 14:56, Ananyev, Konstantin:
> > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > Konstantin, I would like your opinion about the proposal below.
> > > > It is about making on the fly configuration more generic.
> > > > You say it is possible to configure VLAN on the fly,
> > > > and I think we should make it possible for other offload features.
> > >
> > > It would be a good thing, but I don't think it is possible for all of=
floads.
> > > For some of them you still have to stop the queue(port) first.
> > >
> > > Also I am not sure what exactly do you propose?
> > > Is that something like that:
> > > - wipe existing offload bitfileds from rte_eth_rxmode (already done b=
y Shahaf)
> > > - Instead of uint64_t offloads inside both  rte_eth_rxmode and te_eth=
_rxconf
> > >   Introduce new functions:
> > >
> > > int rte_eth_set_port_rx_offload(portid, uint64_t offload_mask);
> > > int rte_eth_set_queue_rx_offload(portid, queueid, uint64_t offload_ma=
sk);
> Would be useful to have a valid mask here, to indicate what bits to use.
> That way, you can adjust one bit without worrying about what other bits
> you may change in the process. There are probably apps out there that
> just want to toggle a single bit on, and off, at runtime while ignoring
> others.
> Alternatively, we can have set/unset functions which enable/disable
> offloads, based on the mask.

My thought was  that people would do:

uint64_t offload =3D rte_eth_get_port_rx_offload(port);
offload |=3D RX_OFFLOAD_X;
offload &=3D ~RX_OFFLOAD_Y;
rte_eth_set_port_rx_offload(port, offload);

In that case, I think we don't really need a mask.

>=20
> > >
> > > uint64_t rte_eth_get_port_rx_offload(portid);
> > > uint64_t rte_eth_set_queue_rx_offload(portid, queueid);
> s/set/get/
> > >
> > > And add new fileds:
> > > rx_offload_port_dynamic_capa
> > > rx_offload_queue_dynamic_capa
> > > inside rte_eth_dev_info.
> > >
> > > And it would be user responsibility to call set_port/queue_rx_offload=
()
> > > somewhere before dev_start() for static offloads.
> > > ?
> >
> > Yes exactly.
> >
> > > If so, then it seems reasonable to me.
> >
> > Good, thank you
> >
> >
> Sorry I'm a bit late to the review, but the above suggestion of separate
> APIs for enabling offloads, seems much better than passing in the flags
> in structures to the existing calls. From what I see all later revisions
> of this patchset still use the existing flags parameter to setup calls
> method.
>=20
> Some advantages that I see of the separate APIs:
> * allows some settings to be set before start, and others afterwards,
>   with an appropriate return value if dynamic config not supported.
> * we can get fine grained error reporting from these - the set calls can
>   all return the mask indicating what offloads could not be applied -
>   zero means all ok, 1 means a problem with that setting. This may be
>   easier for the app to use than feature discovery in some cases.
> * for those PMDs which support configuration at a per-queue level, it
>   can allow the user to specify the per-port settings as a default, and
>   then override that value at the queue level, if you just want one queue
>   different from the rest.

I think we all in favor to have a separate API here.
Though from the discussion we had at latest TB, I am not sure it is doable
in 17.11 timeframe.
Konstantin