[EXT] Re: [PATCH v3 21/22] pdcp: add thread safe processing

[Anoob Joseph]

Hi Stephen,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Stephen Hemminger <stephen at networkplumber.org>
> Sent: Thursday, May 25, 2023 12:02 AM
> To: Anoob Joseph <anoobj at marvell.com>
> Cc: Thomas Monjalon <thomas at monjalon.net>; Akhil Goyal
> <gakhil at marvell.com>; Jerin Jacob Kollanukkaran <jerinj at marvell.com>;
> Konstantin Ananyev <konstantin.v.ananyev at yandex.ru>; Bernard
> Iremonger <bernard.iremonger at intel.com>; Volodymyr Fialko
> <vfialko at marvell.com>; Hemant Agrawal <hemant.agrawal at nxp.com>;
> Mattias Rönnblom <mattias.ronnblom at ericsson.com>; Kiran Kumar
> Kokkilagadda <kirankumark at marvell.com>; dev at dpdk.org; Olivier Matz
> <olivier.matz at 6wind.com>
> Subject: [EXT] Re: [PATCH v3 21/22] pdcp: add thread safe processing
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Wed, 24 May 2023 21:31:15 +0530
> Anoob Joseph <anoobj at marvell.com> wrote:
> 
> > From: Volodymyr Fialko <vfialko at marvell.com>
> >
> > PDCP state has to be guarded for:
> >
> > - Uplink pre_process:
> >     - tx_next atomic increment
> >
> > - Downlink pre_process:
> >     - rx_deliv - read
> >
> > - Downlink post_process:
> >     - rx_deliv, rx_reorder, rx_next - read/write
> >     - bitmask/reorder buffer - read/write
> >
> > When application requires thread safe processing, the state variables
> > need to be updated atomically. Add config option to select this option
> > per entity.
> >
> > Signed-off-by: Anoob Joseph <anoobj at marvell.com>
> > Signed-off-by: Volodymyr Fialko <vfialko at marvell.com>
> 
> NAK
> Conditional locking is a bad design pattern.
> It leads to lots of problems, and makes it almost impossible for analysis tools.

[Anoob] With PDCP (& most other protocols), we have to update the states atomically. Application designers would have a choice of either use single thread or do multi-thread processing. If the library is designed for multi-thread and if application uses only single thread, then there would be unnecessary overheads from library. If library sticks to single-thread and if application needs more threads for scaling, then again it would become a library issue.

Is your issue with providing such an option or is it about how it is implemented? IPsec also has a similar challenge and similar per SA configuration is provided in lib IPsec as well.