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, 6 Jun 2023 19:31:56 +0300
From:   Vladimir Oltean <vladimir.oltean@nxp.com>
To:     Jamal Hadi Salim <jhs@mojatatu.com>
Cc:     netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Cong Wang <xiyou.wangcong@gmail.com>,
        Jiri Pirko <jiri@resnulli.us>,
        Vinicius Costa Gomes <vinicius.gomes@intel.com>,
        linux-kernel@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
        Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>,
        Peilin Ye <yepeilin.cs@gmail.com>,
        Pedro Tammela <pctammela@mojatatu.com>
Subject: Re: [PATCH RESEND net-next 0/5] Improve the taprio qdisc's
 relationship with its children
Message-ID: <20230606163156.7ee6uk7jevggmaba@skbuf>
References: <20230602103750.2290132-1-vladimir.oltean@nxp.com>
 <CAM0EoMnqscw=OfWzyEKV10qFW5+EFMd5JWZxPSPCod3TvqpnuQ@mail.gmail.com>
 <20230605165353.tnjrwa7gbcp4qhim@skbuf>
 <CAM0EoM=qG9sDjM=F6D=i=h3dKXwQXAt1wui8W_EXDJi2tijRnw@mail.gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAM0EoM=qG9sDjM=F6D=i=h3dKXwQXAt1wui8W_EXDJi2tijRnw@mail.gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?Q5TyWCX1vwV+9VAlj8qNAH/6aOVFjmkIKSmycUjXx0G6KzDPwDu5Ni0XPxZC?=
 =?us-ascii?Q?3U7XvpIkswF34IhjEw9poAtuKRD1tFkm4isYZ4grej3MUWa5DKh7HQHdUprS?=
 =?us-ascii?Q?QAyq9o7S8jxoL5bskI1+jUAjBk9TeHBWIYBOZM9ehMc7EsM7AYN4p3Q04LE+?=
 =?us-ascii?Q?kIK91ROPD8zTXrHaRawzpyynYJzkQXFpzmSx7X1oHYTvpDsI9flky6IDb/Fe?=
 =?us-ascii?Q?V+h+8CsMb3FDiQhGEObkkDvnT3K65vadzk8I7whZQdfgwnOpqCi92waNyXrY?=
 =?us-ascii?Q?Dq0xC2G796Hk6M1MjRPQRwuWgI/7YL/cEB6upNj2Svz8CvJGPgSgynHJPt7C?=
 =?us-ascii?Q?h37sDnllN3Y9mi8GSrcqbkzijQ8Bv/z3uQdeeONczZRfxY/Zt8mQEkJcfpIG?=
 =?us-ascii?Q?Kt4a40KaYL6MTjc+JDfbIGyPL2FJiFO2xLwZwNlQyVtV+FQlhF9+xUnCA+dR?=
 =?us-ascii?Q?f4RG4jOCpcLibMU3javwSazh9I/b33VZgH2YoezmXcBTgcpxcOnd8UijOxCw?=
 =?us-ascii?Q?mJHuy6hNSI6rWG/GKbJi3a1UiRFqKML8MtRd68r/JWiWfFeBoy6VeoIg+MZD?=
 =?us-ascii?Q?qNLw+OJqLPic33sqymKQ64UhjfXwrQko1sMe4llhb5dkVjmRfDDFA+D3kdon?=
 =?us-ascii?Q?OHfUx2fnaTVIRrArivsuVGoIquREFnh9c62Np07fFjEWiEjnr7DV5MmvQ5NO?=
 =?us-ascii?Q?uC7AAnFEE0vPMg0OGs2e/dEDdkwypAm5SwjQvcdJYeOYNduz2MSlEwbqCz39?=
 =?us-ascii?Q?6M7v9UqDJGZHSwe6VDc1vxYNJs3FIE/9eCxwa/tMgUQMyzA46aYC7b0sReFU?=
 =?us-ascii?Q?UCiFUzfRQFPuq/yB0X14GfBIQ85s7pUTLPiV1/iMoF2dkuXd66IG1JmLGJ8X?=
 =?us-ascii?Q?Y3gttLCgTlLvgKHVP+f9SZa8wNlwsvD8j3tbiwyIGlu24TH5bPimQQqNaFQ7?=
 =?us-ascii?Q?GhLdkE9OSFFZcfvFAsO4vcA3SK4hG4gZfJy8Sx25EYcGmK3p0EjAW9Pgh7Ve?=
 =?us-ascii?Q?PZnJ++PAke6usL51IKDlG6sNkp7eilPVuVMf7PaJwqkLtYVCJP0ZtoPzrmRV?=
 =?us-ascii?Q?d/lu9GEpHKKQBweHwZ7xXPRWC5shW4P3Fq0AQVt9sQfx0gR/fRZSZLr2NuR+?=
 =?us-ascii?Q?iR+GeYwzlD+0J3NG7BDlYJySeHfAcaZ+jaHEEpK0sEj2F+zwQ8WZxN9y/3uD?=
 =?us-ascii?Q?DHCh5nMK0qgvX2vOgNdab079pqK6gFy7Gafv1OvPf7PEOBrb1YGT9U//iF0V?=
 =?us-ascii?Q?L+z42JsrEc/KOUmG2+bmnzLyxuCd9puIQbKWE71vvkBsws/lpDofUL0c8+3o?=
 =?us-ascii?Q?6HxEWOzZowqMHO49zsJ3E8EhEjd9gsV7JdSWHUi/0vIOB0VFH9GjL7Z2qOd0?=
 =?us-ascii?Q?d4jlW6OaittGe14lED5fdKAnOjtUG3We4bQhAlV58qKFK73n0ma8N4xjnSi/?=
 =?us-ascii?Q?adcao0t9smZSlt5cfhfhAFFjSpfYoqImwVmh8RJJqot05qXcTSIiEeHQffO5?=
 =?us-ascii?Q?0In3K0EcxJ3wy3s8pYzO71EeT2IGs4iM1BqqTSJz6AeqFCYrpOGA5hc0+UBo?=
 =?us-ascii?Q?CNwm9x+QKKr7C7o4Jb0YEko3UzxrAHTF4Rw+FtxrhXCBEDFbnXbHRxgwe5z4?=
 =?us-ascii?Q?mQ=3D=3D?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 7a49a222-fc56-485e-e61c-08db66ab8d66
X-MS-Exchange-CrossTenant-AuthSource: AM0PR04MB6452.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Jun 2023 16:32:00.2641
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 1DUlQ956gPjsQwk/hAXTEPnrOg/eiqi+P5BcBgi3PICtYcojXBsbCix7NL4dWAQhpzULOe3F0bToEzEICSZ1jg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8PR04MB9046
X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,SPF_HELO_PASS,
        T_SCC_BODY_TEXT_LINE,T_SPF_PERMERROR,URIBL_BLOCKED autolearn=no
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Jun 06, 2023 at 11:39:32AM -0400, Jamal Hadi Salim wrote:
> 1)Just some details become confusing in regards to offload vs not; F.e
> class grafting (taprio_graft()) is de/activating the device but that
> seems only needed for offload. Would it not be better to have those
> separate and call graft_offload vs graft_software, etc? We really need
> to create a generic document on how someone would write code for
> qdiscs for consistency (I started working on one but never completed
> it - if there is a volunteer i would be happy to work with one to
> complete it).

I would be a happy reader of that document. I haven't studied whether
dev_deactivate() and dev_activate() are necessary for the pure software
data path, where the root taprio is also directly attached to the netdev
TXQs and that fact doesn't change across its lifetime.

> 2) It seems like in mqprio this qdisc can only be root qdisc (like
> mqprio)

so far so good

> and you dont want to replace the children with other types of
> qdiscs i.e the children are always pfifo? i.e is it possible or
> intended for example to replace 8001:x with bfifo etc? or even change
> the pfifo queue size, etc?

no, this is not true, why do you say this?

> 3) Offload intention seems really to be bypassing enqueue() and going
> straigth to the driver xmit() for a specific DMA ring that the skb is
> mapped to. Except for the case where the driver says it's busy and
> refuses to stash the skb in ring in which case you have to requeue to
> the appropriate child qdisc/class. I am not sure how that would work
> here - mqprio gets away with it by not defining any of the
> en/de/requeue() callbacks

wait, there is a requeue() callback? where?

> - but likely it will be the lack of requeue that makes it work.

Looking at dev_requeue_skb(), isn't it always going to be requeued to
the same qdisc it was originally dequeued from? I don't see what is the
problem.

My understanding of the offload intention is not really to bypass dequeue()
in general and as a matter of principle, but rather to bypass the root's
taprio_dequeue() specifically, as that could do unrelated work, and jump
right to the specific child's dequeue().

The child could have its own complex enqueue() and dequeue() and that is
perfectly fine - for example cbs_dequeue_soft() is a valid child dequeue
procedure - as long as the process isn't blocked in the sendmsg() call
by __qdisc_run() processing packets belonging to unrelated traffic
classes.