Re: [PATCH RESEND net 3/7] net: hns3: Resolved the issue that the debugfs query result is inconsistent.

[Jijie Shao]

on 2024/11/14 8:31, Jakub Kicinski wrote:
> On Wed, 13 Nov 2024 13:59:32 +0800 Jijie Shao wrote:
> > inconsistent：
> > Before this modification,
> > if the previous read operation is stopped before complete, the buffer is not released.
> > In the next read operation (perhaps after a long time), the driver does not read again.
> > Instead, the driver returns the bufffer content, which causes outdated data to be obtained.
> > As a result, the obtained data is inconsistent with the actual data.
> I think the word "stale" would fit the situation better.
>
> > In this patch, ppos is used to determine whether a new read operation is performed.
> > If yes, the driver updates the data in the buffer to ensure that the queried data is fresh.
> > But, if two processes read the same file at once, The read operation that ends first releases the buffer.
> > As a result, the other read operation re-alloc buffer memory. However, because the value of ppos is not 0,
> > the data is not updated again. As a result, the queried data is truncated.
> >
> > This is a bug and I will fix it in the next version.
> Let's say two reads are necessary to read the data:
>
>   reader A                  reader B
>    read()
>     - alloc
>     - hns3_dbg_read_cmd()
>                             read()
>                             read()
>                             read(EOF)
>                              - free
>    read()
>     - alloc
>     - hns3_dbg_read_cmd()
>    read(EOF)
>     - free
>
> The data for read A is half from one hns3_dbg_read_cmd() and half from
> another. Does it not cause any actual inconsistency?
>
> Also, just to be sure, it's not possible to lseek on these files, right?

The patch of this version has the following problems:
 reader A                       reader B
  read()
   - alloc
   - *ppos == 0
    - hns3_dbg_read_cmd()
                                read()
                                read()
                                read(EOF)
                                 - free
  read()
   - alloc
   - *ppos != 0
  read(EOF)
   - free
   
if reader B free the buffer, reader A will alloc a new buffer again,
but *ppos != 0, so hns3_dbg_read_cmd() will not be called.
So reader A cannot get the left data.

I plan to introduce the "need_update" variable in the next version,
with the default value is false. Run the alloc command to change the value to true：
 reader A                           reader B
  read()
   - need_update = false
   - alloc
      - need_update = true
   - *ppos == 0 || need_update
      - hns3_dbg_read_cmd()
                                    read()
                                    read()
                                    read(EOF)
                                    - free
  read()
   - alloc
     - need_update = true
   - *ppos == 0 || need_update
     - hns3_dbg_read_cmd()
  read(EOF)
   - free
So,  after reader A alloc a new buffer again, need_update will set to true.
hns3_dbg_read_cmd() will be called again to update data.

But there's still a problem：
The data for reader A is half from one hns3_dbg_read_cmd() and half from another.
However, due to the short interval between calls to hns3_dbg_read_cmd(),
and the fact that users do little to do like so, this problem is relatively acceptable.

We're also trying to fix the problem completely.
For example, an independent buffer is allocated for each reader:
 reader A                   reader B
  read()                    read()
  - alloc                   - alloc
    - hns3_dbg_read_cmd()    -hns3_dbg_read_cmd()
  read()                    read()
  read()                    read()
  read(EOF)                 read(EOF)
  - free                    - free
 
But, driver needs to maintain the mapping between the buffer and file.
And if the reader exits before read EOF, a large amount of memory is not free:
 reader
  read()
  - alloc
    - hns3_dbg_read_cmd()
  read()
  read()
  == reader exit ==
Maybe it's not a good way

As for lseek, driver needs to call lseek at the right time to reread the data.
 reader A                           reader B
   read()
   alloc
   hns3_dbg_read_cmd()
   *ppos == 0
   read()
                                    read()
                                    read()
                                    read(EOF)
                                    - free
   alloc
   hns3_dbg_read_cmd()
   - *ppos != 0
   - lseek()
   - *ppos == 0
   reread()
   read()
   read(EOF)
   free

I can't find any examples of similar implementations.
I'm not sure if there's a suitable lseek interface that the driver can call directly.


Another way is seq_file, which may be a solution,
as far as I know, each seq_file has a separate buffer and can be expanded automatically.
So it might be possible to solve the problem
But even if the solution is feasible, this will require a major refactoring of hns3 debugfs

Thanks
Jijie Shao