netdev - Re: [PATCH v2 net-next 06/12] qed: Add LL2 slowpath handling

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: <CY1PR0701MB20123E0FC36400232910B65888710@CY1PR0701MB2012.namprd07.prod.outlook.com>
Date:   Fri, 6 Oct 2017 05:09:13 +0000
From:   "Kalderon, Michal" <Michal.Kalderon@...ium.com>
To:     David Miller <davem@...emloft.net>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        "dledford@...hat.com" <dledford@...hat.com>,
        "Elior, Ariel" <Ariel.Elior@...ium.com>
Subject: Re: [PATCH v2 net-next 06/12] qed: Add LL2 slowpath handling

From: linux-rdma-owner@...r.kernel.org <linux-rdma-owner@...r.kernel.org> on behalf of David Miller <davem@...emloft.net>

>From: "Kalderon, Michal" <Michal.Kalderon@...ium.com>
>Date: Thu, 5 Oct 2017 20:27:22 +0000
>
>> The spinlock is required for the case that rx buffers are posted
>> from a different thread, where it could be run simultaneously to the
>> rxq_completion.
>
>This only brings us back to my original argument, if the lock is
>necessary in order to synchronize with those paths, how can you
>possible drop the lock safely here?
>
>Is it because you re-read the head and tail pointers of the queue each
>time around the loop?

It's safe to drop the lock here because the implementation of the queue (qed_chain)
maintains both a consumer pointer (tail) indices and producer pointer(head).
The main loop reads  the FWs value and stores it as the "new cons"
and traverses the queue only until it reaches the FWs consumer value. 
The post function adds more buffers to the end of the queue and updates the 
producer. They will not affect the consumer pointers. So posting of buffers
doesn't affect the main loop.

The resources that are protected by the lock and accessed inside the loop
and from post-buffers are three linked-lists, free-descq, posting_descq and
active_descq, their head and tail are  read on every access
(elements are removed and moved between the lists).

Following this discussion, it looks like there was no need to take the lock in the
outer function, but only around the places that access these lists, this is a delicate
change which affects the ll2 clients (iscsi,fcoe,roce,iwarp). As this series is rather
large as it is and is intended for iWARP, please consider taking this one as it is. 
Since it doesn't change existing functionality and doesn't introduce risk to other
components. 

thanks,
Michal

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html